dispatcher: Delay fd activation until the next itertion of the event loop. #11750
Conversation
…ted by runtime feature "envoy.reloadable_features.activate_fds_next_event_loop". Signed-off-by: Antonio Vicente <avd@google.com>
mattklein123
left a comment
There was a problem hiding this comment.
Thanks this is awesome. Just a few small comments.
/wait
| // Latched "envoy.reloadable_features.activate_fds_next_event_loop" runtime feature. If true, fd | ||
| // events scheduled via activate are evaluated in the next iteration of the event loop after | ||
| // polling and activating new fd events. | ||
| bool activate_fd_events_next_event_loop_; |
| const timeval zero_tv{}; | ||
| event_add(&raw_event_, &zero_tv); |
There was a problem hiding this comment.
Can you add some comments here and in scheduleCallbackCurrentIteration about how these libevent invocations differ? It's not completely clear to me and since you have spent so much time looking at libevent source it would be good to leave a comment trail.
There was a problem hiding this comment.
Added comments.
Originally I read your request as documenting the stages of execution of the event loop. I wasn't sure where a comment like this one would belong:
Rough summary of operations that libevent performs in each event loop iteration, in order:
- Calculate the poll timeout by comparing the current time to the deadline of the closest
timer (the one at head of the priority queue). - Run registered prepare callbacks.
- Poll for fd events using the closest timer as timeout, add active fds to the work list.
- Run registered "check" callbacks.
- Check timer deadlines against current time, add expired timers to the work list.
- Execute items in the work list until the list is empty. Note that additional work
items could be added to the work list during execution of this step. - Goto 1.
Additional work items can be added to the work list so they execute in the current iteration of
the event loop by using one of the mechanisms below. Note that there are no ordering guarantees
when the mixing the mechanisms below.
- Event::Dispatcher::post(cb), which execute as a group.
- Event::Dispatcher::deferredDelete(object), which execute as a group together with
DeferredTaskUtil callbacks. - Event::DeferredTaskUtil::deferredRun(dispatcher, cb), which execute as a group together with
deferredDelete. - Event::SchedulableCallback::scheduleCallbackCurrentIteration(), which executes independently.
- Event::FileEvent::activate() if "envoy.reloadable_features.activate_fds_next_event_loop"
runtime feature is disabled. - Event::Timer::enableTimer(0) if "envoy.reloadable_features.activate_timers_next_event_loop"
runtime feature is disabled.
Event::FileEvent::activate and Event::SchedulableCallback::scheduleCallbackNextIteration are
implemented as libevent timers with a deadline of 0, so those events are moved to the work list
while checking for expired timers during step 5.
Event execution order, based in the order in which items are added to the work list:
0. Events added via event_active prior to the start of the event loop (in tests)
- Fd events
- Timers, FileEvent::activate and SchedulableCallback::scheduleCallbackNextIteration
- Same iteration work items described above, including Event::Dispatcher::post callbacks
There was a problem hiding this comment.
Thanks this is amazing detail. It would be nice to capture this somewhere but up to you where you want to put it.
Perhaps either a small mardown file here: https://github.com/envoyproxy/envoy/tree/master/source/docs
Or here? https://github.com/envoyproxy/envoy/blob/master/source/common/event/libevent_scheduler.h
wdyt? Feel free to do this later as a follow up if you want.
There was a problem hiding this comment.
libevent_scheduler.h seems like a good place for the comment.
I'll add the comment block in the followup PR that introduces "envoy.reloadable_features.activate_timers_next_event_loop"
Signed-off-by: Antonio Vicente <avd@google.com>
|
CI failure looks legit. /wait |
Signed-off-by: Antonio Vicente <avd@google.com>
….activate_fds_next_event_loop (#14055) Feature flag was introduced in PR #11750 on 2020-06-27. Would be good to obsolete in preparation to changes to activate/setEnable behavior which should make activate behavior more intuitive and simplify edge trigger emulation and userspace file event implementations. Signed-off-by: Antonio Vicente <avd@google.com>
….activate_fds_next_event_loop (envoyproxy#14055) Feature flag was introduced in PR envoyproxy#11750 on 2020-06-27. Would be good to obsolete in preparation to changes to activate/setEnable behavior which should make activate behavior more intuitive and simplify edge trigger emulation and userspace file event implementations. Signed-off-by: Antonio Vicente <avd@google.com>
….activate_fds_next_event_loop (envoyproxy#14055) Feature flag was introduced in PR envoyproxy#11750 on 2020-06-27. Would be good to obsolete in preparation to changes to activate/setEnable behavior which should make activate behavior more intuitive and simplify edge trigger emulation and userspace file event implementations. Signed-off-by: Antonio Vicente <avd@google.com> Signed-off-by: Qin Qin <qqin@google.com>
Commit Message: dispatcher: Delay fd activation until the next itertion of the event loop.
Additional Description: Processing injected fd events in the same loop they are generated can result in high-throughput connections proxying data multiple times per event loop iteration, effectively starving other connections and increasing small request latency.
Risk Level: high, changes to event loop scheduling behavior and fd event delivery
Testing: unit
Docs Changes: n/a
Release Notes: n/a
Runtime guard: envoy.reloadable_features.activate_fds_next_event_loop