Fix deboucing hight cpu usage

[vemoo]

This are the changes

• Replace Sender in WatchTimer and Reciever with BinaryHeap in ScheduleWorker with a VecDeque shared behind a Mutex
  • BinaryHeap is not needed because the events are already sorted
  • Share the events collection so that ignores can remove the event, and are not acumulated for the worker to process
• Add tigger for stopping to avoid waking the worker every time a new event arrives, since we know no events should fire yet

[passcod]

I'll let @dfaust review more in depth. I've read the diff and tested some of it and it seems sound, but I don't feel confident judging much more than that!

One tiny stylistic nitpick, though: given the ScheduleWorker::new method does nothing more than pass arguments verbatim, I think it should rather go away altogether and the caller can just create using the ScheduleWorker { ... } syntax. That makes it a bit clearer that nothing else is going on.

[dfaust]

Looks good to me. But I have to admit, I hadn't looked at that code in a long time. What confused me was the variable stop_trigger. It suggests a connection to the stopped variable (for stopping the thread). But it's really a trigger for waiting until the next event in the queue should be processed. How about renaming it to debounce_event_trigger?

[vemoo]

@dfaust Not exactly. If we end up here: https://github.com/vemoo/notify/blob/ac6430f9d95e33371ef82660528596598c8933b6/src/debounce/timer.rs#L115-L119 it's because we know when the next event should fire, and we also know that there can't be an event before that. I initially thought of just using thread::sleep but I realized that when stopping we could end up unnecessarily waiting to fire the next event. stop_trigger will only fire once when stopping, the rest of the time it'll timeout waiting.

When we don't yet know when the next event is we end up here:
https://github.com/vemoo/notify/blob/ac6430f9d95e33371ef82660528596598c8933b6/src/debounce/timer.rs#L121-L122 , the new_event_trigger is notified of a new event, we wake up and check when it should be fired, and now we should have a next_when and we can wait to send the event.

[dfaust]

@vemoo Yes, you're right.

Maybe changing the comments will make things easier to understand.

- // wait to send event
+ // wait for stop notification or timeout to send next event

- // wait for new event
+ // wait for new event (a new event notification will also be issued when stopping)

[vemoo]

a new event notification will also be issued when stopping

That won't happen unless the delay is really small, because the fire_due_events will see a new event but it won't be due yet, so it would continue and break the loop.

[dfaust]

a new event notification will also be issued when stopping

That won't happen unless the delay is really small, because the fire_due_events will see a new event but it won't be due yet, so it would continue and break the loop.

What I meant was that both stop_trigger and new_event_trigger are notified when dropping the WatchTimer. So new_event_trigger doesn't just wait for new events.

Anyways, I'm going to merge this now because it's a great improvement. Thank you very much!

[vemoo]

Yeah, I just realised that

[passcod]

Thank you! I'll go release this later today.