Skip to content

Adds a new_with_delay constructor to PollWatcher#34

Merged
passcod merged 1 commit intonotify-rs:masterfrom
TyOverby:poll-watcher-with-delay
Oct 24, 2015
Merged

Adds a new_with_delay constructor to PollWatcher#34
passcod merged 1 commit intonotify-rs:masterfrom
TyOverby:poll-watcher-with-delay

Conversation

@TyOverby
Copy link
Contributor

Currently, PollWatcher eats 100% of CPU on one of my cores because it is sitting in a tight loop. This commit includes an option to let the thread sleep once-per-iteration.

@passcod
Copy link
Member

passcod commented Oct 20, 2015

I'm wary of extending the polling watcher API for a specific purpose. I would however accept a pull request that modified PollWatcher's default constructor with a researched change.

We'd need answers to these questions:

  • Does the problem you have occur on other platforms? (I assume from your previous issue that your platform is OS X.)
  • What's the minimum delay sufficient for a fix?
  • What's the maximum delay that is acceptable?

Then taking into account these, hardcode a reasonable value in. That would then let every user on every platform take advantage of the fix by default.

@TyOverby
Copy link
Contributor Author

Does the problem you have occur on other platforms? (I assume from your previous issue that your platform is OS X.)

It happens on both platforms that I've tested it on (OSX and Windows). I think the reason that it shows up is that I'm only watching a single file, so the program doesn't spend much time in the FS syscalls.

What's the minimum delay sufficient for a fix?

Probably 1ms.

On my laptop (OSX):

sleep_ms(0): 100% CPU
sleep_ms(1):   6% CPU
sleep_ms(10):  1% CPU

I'll test my Windows and Linux machines when I get home and see if they show similar behavior.

What's the maximum delay that is acceptable?

Not sure if we need to worry about this.

Then taking into account these, hardcode a reasonable value in. That would then let every user on every platform take advantage of the fix by default.

This is how things work right now. I made it so that the default constructor uses a default-value of 20ms (I could change that to 1ms if that ends up being reasonable for every system.

@passcod
Copy link
Member

passcod commented Oct 20, 2015

This is how things work right now.

Indeed, sorry I must have missed this.

@passcod
Copy link
Member

passcod commented Oct 20, 2015

Not sure if we need to worry about this.

With these times, probably not. I would start being worried if the delay would be a human-noticeable one like, say, 200ms. 10 or 20ms seems like a good choice, though.

@TyOverby
Copy link
Contributor Author

Yeah, I think 10ms would be good. I'll make this change right now.

@TyOverby
Copy link
Contributor Author

@passcod Is there anything else that you want to cover, or is this mergeable?

@passcod
Copy link
Member

passcod commented Oct 22, 2015

Yes, sorry for the delay. The naming style throughout the stdlib would prefer ::with_delay() to ::new_with_delay(). Apart from that, LGTM!

@passcod
Copy link
Member

passcod commented Oct 22, 2015

@blaenk Do you want to have a quick look over this PR to see if I've missed anything?

@TyOverby TyOverby force-pushed the poll-watcher-with-delay branch from 85fea99 to c18ac61 Compare October 22, 2015 21:57
passcod added a commit that referenced this pull request Oct 24, 2015
Adds a ::with_delay() constructor to PollWatcher
@passcod passcod merged commit d54e8b6 into notify-rs:master Oct 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants