Add facilities for signal throttling and debouncing #45
Conversation
There was a problem hiding this comment.
Overall, a good step in the right direction.
I do believe that using Debouncer and Throttler objects would go a long way to simplify the API though.
In the end what basically happens is that a closure is constructed inside the function, which behaves just like an object, it just uses captured variables as members. (fun fact: this is actually how to construct an "object" in LISP). So this could just as well happen outside the function, so the Signal itself doesn't have to be concerned with this.
There are also a few implementation issues that I've noted.
src/kdbindings/signal.h
Outdated
| slot(args...); | ||
|
|
||
| shouldWait = true; | ||
| std::thread([&shouldWait, throttleDelay]() { |
There was a problem hiding this comment.
This is a very expensive way to wait.
What should be a lot easier is to just save the current time-stamp whenever the slot is actually called.
Then you can calculate whether you should wait by just subtracting the current time from the stored time.
If that delta is smaller than your timeout, it should throttle.
No need for a whole thread.
There was a problem hiding this comment.
I'm just noticing, you're already doing exactly that in the connectWithDebouncing case.
Basically, just do what you're doing there. Though the current implementation is leading, instead of trailing, which I think is an okay default to stick with, as then we're not dropping events.
| } | ||
|
|
||
| // Connect a callback function with Debouncing | ||
| Private::GenerationalIndex connectWithDebouncing(std::function<void(Args...)> const &slot, int interval) |
There was a problem hiding this comment.
If I read the description of throttler and debouncer correctly, this here isn't actually a Debouncer, but a trailing Throttler.
If a 100ms Debouncer is called 20 times, every time with a delay of 50ms in-between it should only execute the slot once - 100ms after the last of the twenty signals is received.
The current implementation would call the slot 10 times, after every second signal, where the timeout of 100ms expires each time.
That's what a throttler should do if my understanding is correct.
Please adjust the tests to differentiate between debouncer and throttler.
There was a problem hiding this comment.
Implementation note:
I believe in our current implementation we cannot easily implement a debouncer (or potentially even a trailing throttler).
As a debouncer needs to emit a signal after the timeout expires. However, it cannot do that, as it's not in control of the currently running thread.
The implementation in KDToolBox uses QTimer to achieve this.
However, QTimer relies on Qt's event-loop to be able to run on the currently active thread:
In multithreaded applications, you can use QTimer in any thread that has an event loop. To start an event loop from a non-GUI thread, use QThread::exec(). Qt uses the timer's thread affinity to determine which thread will emit the timeout() signal. Because of this, you must start and stop the timer in its thread; it is not possible to start a timer from another thread.
As we don't have an event-loop to queue even events on we use ConnectionEvaluator as a substitute. So any Debouncer would need a ConnectionEvaluator to which it could queue the emissions.
There was a problem hiding this comment.
Ah indeed it is challenging to implement debouncer and throttler in our current implementation , so basically if i am understanding clearly, we need a way that whenever a signal is received, we schedule a task to run after the debounce interval. If another signal is received before the interval expires, we cancel/remove the previously scheduled task and schedule a new one. And something similar with throttler ? We will need a mechanism to queue up the task/slots for debouncer and throttler, or we may use ConnectionEvaluator but need to look once the patch for it lands.
tests/signal/tst_signal.cpp
Outdated
| signal.emit(1); | ||
| REQUIRE(count == 0); // Debouncing interval hasn't passed | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); // Wait for debouncing interval |
There was a problem hiding this comment.
We definitely do not want to be waiting and rely on time this much in a test...
There are basically two things that are problematic:
- we have to wait 100ms every time we run this test, if we want to add more debouncing tests, this means more waiting for tests to finish.
- If the program is interrupted by the OS between
connectWithDebouncingandsignal.emitfor more than 100ms (or whatever time we choose here), the test will fail as the timer will have expired before we thought. With such a long delay, this is highly unlikely, but for reasons against such a long delay, see1., the shorter we choose this delay, the more likely this issue will be however.
The only real solution I can think of is mocking/faking time for our testing.
Generally this means to include some facility to override time, e.g. by having an optional<function<>> that we can set during testing to return any time we want, or by providing means to finish timers early.
A more high-level approach may be something like libfaketime (see Zeno's Edu-Report "Speeding up time based unit tests with libfaketime").
Though this is likely overkill for our use-case.
There was a problem hiding this comment.
Ah yes, that's true faking the time for testing could be one option. I'll look in that
phyBrackets
force-pushed
the
throttle_debounce
branch
from
00ddc48
to
eac6199
Compare
|
As this is rather stale now, I'll close the PR for now. |
Implementing #44