dispatcher: Delay fd activation until the next itertion of the event loop.

[antoniovicente]

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

[mattklein123]

Thanks this is awesome. Just a few small comments.

/wait

[mattklein123]

nit: const

[antoniovicente]

Done.

[mattklein123]

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.

[antoniovicente]

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:

1. Calculate the poll timeout by comparing the current time to the deadline of the closest
   timer (the one at head of the priority queue).
2. Run registered prepare callbacks.
3. Poll for fd events using the closest timer as timeout, add active fds to the work list.
4. Run registered "check" callbacks.
5. Check timer deadlines against current time, add expired timers to the work list.
6. 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.
7. 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)

1. Fd events
2. Timers, FileEvent::activate and SchedulableCallback::scheduleCallbackNextIteration
3. Same iteration work items described above, including Event::Dispatcher::post callbacks

[mattklein123]

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.

[antoniovicente]

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"

[mattklein123]

CI failure looks legit.

/wait

[mattklein123]

Nice!