event: split dispatcherimpl into two classes #14533
Conversation
Wrap DispatcherImpl around DispatcherImplBase. This allows writing low-level stuff like timers and file events in terms of the base interface. Signed-off-by: Alex Konradi <akonradi@google.com>
This prevents an extra heap allocation and doesn't affect correctness. Signed-off-by: Alex Konradi <akonradi@google.com>
Signed-off-by: Alex Konradi <akonradi@google.com>
Signed-off-by: Alex Konradi <akonradi@google.com>
antoniovicente
requested review from
mattklein123,
ggreenway and
antoniovicente
| base_scheduler_.registerOnPrepareCallback( | ||
| std::bind(&DispatcherImpl::updateApproximateMonotonicTime, this)); | ||
| base_.updateApproximateMonotonicTime(); | ||
| base_.registerOnPrepareCallback(std::bind(&DispatcherImpl::updateApproximateMonotonicTime, this)); |
There was a problem hiding this comment.
These two calls should be moved to base_'s constructor.
|
|
||
| void DispatcherImpl::updateApproximateMonotonicTimeInternal() { | ||
| approximate_monotonic_time_ = api_.timeSource().monotonicTime(); | ||
| base_.run(type); |
There was a problem hiding this comment.
post callback functionality should move to base_ along with deferred deletion.
The implementation of DispatcherImpl::run should just forward to base_
There was a problem hiding this comment.
I've moved all the functionality into the base. I decided to leave the deferred deletion list in DispatcherImpl since it doesn't need to be in the base, though the need to clear the list before running post callbacks makes for some annoying callback indirection.
There was a problem hiding this comment.
post callback execution happens frequently. The extra expense of this indirection to call into the method that clears the deferred deletion list doesn't make sense.
| @@ -14,7 +14,7 @@ class ScopeTrackedObject; | |||
|
|
|||
| namespace Event { | |||
|
|
|||
| class Dispatcher; | |||
| class DispatcherBase; | |||
There was a problem hiding this comment.
Thinking forward to the scaled timer integration:
Do we expect to add scaled timers to the DispatcherBase or Dispatcher interface?
Not having access to scaled timers in the base interface would severely limit its potential use in more places like Network::ConnectionImpl implementation. How do we see Dispatcher interfaces evolving from here? Or is the only use case for DispatcherBase in the implementation of scaled timers?
There was a problem hiding this comment.
I was imagining them being in the Dispatcher interface. Network::ConnectionImpl would continue to depend on Dispatcher.
I don't have another use case yet for DispatcherBase. We could try transitioning some of the higher-level abstractions like the DNS resolver to depend only on DispatcherBase but if we try to migrate too much we'll just end up with DispatcherBase becoming the new Dispatcher.
| @@ -12,7 +12,7 @@ namespace Envoy { | |||
| // dispatcher at the previously tracked object. | |||
| class ScopeTrackerScopeState { | |||
| public: | |||
| ScopeTrackerScopeState(const ScopeTrackedObject* object, Event::Dispatcher& dispatcher) | |||
| ScopeTrackerScopeState(const ScopeTrackedObject* object, Event::DispatcherBase& dispatcher) | |||
There was a problem hiding this comment.
Could we keep requiring a fully Dispatcher when using this object?
There was a problem hiding this comment.
It gets called from the Timer, which is limited to the DispatcherBase.
Signed-off-by: Alex Konradi <akonradi@google.com>
Signed-off-by: Alex Konradi <akonradi@google.com>
There was a problem hiding this comment.
Thanks for the refactor. I'ld like to understand where the next set of changes will take us. The change feels large enough to warrant some second opinions.
+1 from me. I don't have any conceptual objection to a refactor like this but it's hard for me to understand the purpose. Can you potentially write up a a very small text explanation of the new class hierarchy that you hope to achieve and why the current situation doesn't work?
Also a small comment on the PR itself just to make it easier to review. Thank you!
/wait
| @@ -37,31 +37,180 @@ | |||
| namespace Envoy { | |||
| namespace Event { | |||
|
|
|||
| DispatcherImplBase::DispatcherImplBase(Api::Api& api, TimeSystem& time_system, | |||
There was a problem hiding this comment.
To make this review simpler can you just rename things in place? If you want to move them later that's fine but for now it's hard to tell what is new/old code.
DispatcherBase was introduced in #14446 |
|
Is this PR obsolete? If yes, please close. |
Commit Message: Split DispatcherImpl into two classes
Additional Description:
Wrap DispatcherImpl around DispatcherImplBase. This allows writing
low-level stuff like timers and file events in terms of the base
interface.
Risk Level: medium - nontrivial refactor, though no functional changes expected
Testing: ran affected tests
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
towards #14401
/assign @antoniovicente