-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Throttler / Timer ; use current scheduler #9611
Conversation
63f1b25
to
d0f27bf
Compare
@jfirebaugh I've started on adding delayed scheduling to the Scheduler. I've borrowed some patterns from Akka, but would love your feedback. The idea is to schedule a message for later delivery on the given mailbox with:
Since we need the object to make the
The Throttler is already using this. The actual implementations for Scheduler would use different strategies for the timers. On the Runloop backed schedulers, we can use the existing Timer/Alarm classes. The thread pool would have a dedicated thread/threads for controlling the timers. The scheduler returns a handle in the form of The Note: |
0431ca6
to
4ddc3ca
Compare
I've added two implementations for the timers:
The only RunLoop that doesn't support scheduling a delayed message is the This should be sufficient to continue work on #9576 |
Trying out this branch, I noticed that I overlooked one issue with the way I use timers now. They still get the current Runloop instead of the runloop I intended them to be scheduled on. |
platform/default/thread_local.cpp
Outdated
@@ -58,8 +58,8 @@ void ThreadLocal<T>::set(T* ptr) { | |||
} | |||
} | |||
|
|||
template class ThreadLocal<RunLoop>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removes the template instantiation, but the corresponding code is still there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code that used the template was in RunLoop
, but has since been removed in favor of a ThreadLocal
on Scheduler
include/mbgl/actor/scheduler.hpp
Outdated
|
||
// Set/Get the current Scheduler for this thread | ||
static Scheduler* GetCurrent(); | ||
static void SetCurrent(Scheduler*); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this one protected
so that only subclasses can set the current scheduler?
test/util/throttler.test.cpp
Outdated
// Invoke twice in a row | ||
throttler.invoke(&Test::doIt); | ||
throttler.invoke(&Test::doIt); | ||
loop.runOnce(); // Let the timer do it's thing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo -> "its" ;)
include/mbgl/actor/scheduler.hpp
Outdated
virtual void cancel() = 0; | ||
|
||
virtual bool isFinished() = 0; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have an almost identical class called WorkTask
(and a few others that are similar). Maybe we can reuse/unify those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AsyncRequest
is the most generic interface. It's an interface for objects that cancel themselves when they go out of scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made it extend from AsyncRequest
. However, we need a way to see if the task is completed (for Throttler
) that's why I made it a new class with a isFinished()
method.
class ThreadPoolScheduledTask : public Scheduler::Scheduled { | ||
public: | ||
// The Impl is scheduled on the Runloop of the timer thread | ||
// and uses it to start a timer. When the timer endsm it calls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
src/mbgl/util/throttler.hpp
Outdated
|
||
delegate.invoke(fn, std::forward<Args>(args)...); | ||
}); | ||
pendingInvocation = delegate.invokeDelayed(timeToNextInvocation, fn, std::forward<Args>(args)...); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::move(fn)
} | ||
private: | ||
std::shared_ptr<Fn> fn; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the combination of WorkRequest
/WorkTask
which is very similar to this. Can this be reused here?
wrap( | ||
[&, weakMailbox = std::move(weakMailbox), message = std::move(message)] () mutable { | ||
if (auto mailbox = weakMailbox.lock()) { | ||
mailbox->push(std::forward<std::unique_ptr<Message>>(message)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why forward
instead of move
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the lambda is mutable, this should also protect against double-invocation when message
is empty.
}; | ||
|
||
} // namespace util | ||
} // namespace mbgl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you submit the RunLoop::Get
→ Scheduler::GetCurrent
changes as a separate PR? I'm 👍 on those ones but much less certain of the throttler / timer changes. Maybe we should just rip out the throttler dependency and rethink it entirely.
platform/android/src/run_loop.cpp
Outdated
@@ -200,19 +200,20 @@ Milliseconds RunLoop::Impl::processRunnables() { | |||
} | |||
|
|||
RunLoop* RunLoop::Get() { | |||
return current.get(); | |||
assert(static_cast<RunLoop*>(Scheduler::GetCurrent())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the type too: assert(dynamic_cast<RunLoop*>(Scheduler::GetCurrent()))
(x4).
platform/android/src/run_loop.cpp
Outdated
@@ -4,6 +4,7 @@ | |||
#include <mbgl/util/thread_local.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer used (x2).
include/mbgl/actor/scheduler.hpp
Outdated
virtual void cancel() = 0; | ||
|
||
virtual bool isFinished() = 0; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AsyncRequest
is the most generic interface. It's an interface for objects that cancel themselves when they go out of scope.
include/mbgl/util/run_loop.hpp
Outdated
@@ -62,6 +62,14 @@ class RunLoop : public Scheduler, | |||
push(task); | |||
return std::make_unique<WorkRequest>(task); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: excess whitespace here.
#include <memory> | ||
|
||
namespace mbgl { | ||
|
||
class Mailbox; | ||
class Message; | ||
|
||
/* | ||
A `Scheduler` is responsible for coordinating the processing of messages by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the references to RunLoop
in this comment.
9df6cda
to
b0c3a82
Compare
278526b
to
617759d
Compare
- This doesn't work for asynchronous rendering - see #9611
- This doesn't work for asynchronous rendering - see #9611
- This doesn't work for asynchronous rendering - see #9611
As discussed in chat; closing this and just removing the throttling all together. |
- This doesn't work for asynchronous rendering - see #9611
- This doesn't work for asynchronous rendering - see #9611
- This doesn't work for asynchronous rendering - see #9611
- This doesn't work for asynchronous rendering - see #9611
- This doesn't work for asynchronous rendering - see #9611
- This doesn't work for asynchronous rendering - see #9611
- This doesn't work for asynchronous rendering - see #9611
- This doesn't work for asynchronous rendering - see #9611
- This doesn't work for asynchronous rendering - see #9611
- This doesn't work for asynchronous rendering - see #9611
This PR builds on top of #9728 to make the Timers and throttlers use the current scheduler.