event: Reduce potential for lock contention while executing dispatcher post callbacks. by antoniovicente · Pull Request #14289 · envoyproxy/envoy

antoniovicente · 2020-12-04T22:23:20Z

Risk Level: low, minor code refactor
Testing: n/a, minimal functional changes expected.
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

…r post callbacks. Signed-off-by: Antonio Vicente <avd@google.com>

jmarantz

this looks great; just a couple of nits. Did we wind up finding any evidence of contention?

jmarantz · 2020-12-04T22:25:43Z

source/common/event/dispatcher_impl.cc

+    if (post_callbacks_.empty()) {
+      return;
    }
+    callbacks = std::move(post_callbacks_);


I didn't think std::move defined clearly what state post_callbacks_ would be in after this statement. So I was thinking maybe swap would be better. WDYT?

post_callbacks_ is a list, so I think this is well defined. I could add post_callbacks_.clear(); after the move.

ok that seems like it would de-risk it. From https://en.cppreference.com/w/cpp/utility/move

Unless otherwise specified, all standard library objects that have been moved from are placed in a valid but unspecified state. That is, only the functions without preconditions, such as the assignment operator, can be safely used on the object after it was moved from: ... str.clear(); // OK, clear() has no preconditions

is that better than swap? I'm fine either way.

I did not see anything about std::list's move semantics in https://en.cppreference.com/w/cpp/container/list

std::swap is defined as 3 move assignments. Added the clear and a comment.

Instead of the clear(), I think it would be better to ASSERT(post_callbacks_.empty()). Then if something gets messed up and it somehow became a copy instead of a move (due to c++-shenanigans or something) we'll notice quickly, instead of callbacks not getting run.

Just jumping in here... what about:

std::list<std::function<void()>> callbacks = [this]() { Thread::LockGuard lock(post_lock_); return std::exchange(post_callbacks_, {}); }();

or some variation using std::exchange. The pattern of using exchange is mentioned in reference to an event Dispatcher class in an article on fluentcpp.com.

I went through the article. I think the most relevant section is the "Why not just move?". The issue of expressing the "empty after move" constraint is addressed here by means of the ASSERT(post_callbacks_,empty());

jmarantz · 2020-12-04T22:26:54Z

source/common/event/dispatcher_impl.cc

+  }
+  // It is important that the execution and deletion of the callback happen while post_lock_ is not
+  // held. Either the invocation or destructor of the callback can call post() on this dispatcher.
+  while (!callbacks.empty()) {


is there some reason not to write this as:

for (auto& callback : callbacks) { callback(); }

Differences in deletion order for the callback objects.

good point. add that to the comment?

Added a comment and test.

I also wanted to test yield behavior, but it turns out apparently ordering of schedulable callback scheduling is not deterministic so the yield behavior is not testable.

Signed-off-by: Antonio Vicente <avd@google.com>

jmarantz

@envoyproxy/senior-maintainers ptal

…ocking Signed-off-by: Antonio Vicente <avd@google.com>

ggreenway

I like the change; much better to not lock/unlock the mutex for each callback separately.

ggreenway · 2020-12-07T22:09:37Z

source/common/event/dispatcher_impl.cc

+    if (post_callbacks_.empty()) {
+      return;
    }
+    callbacks = std::move(post_callbacks_);


Instead of the clear(), I think it would be better to ASSERT(post_callbacks_.empty()). Then if something gets messed up and it somehow became a copy instead of a move (due to c++-shenanigans or something) we'll notice quickly, instead of callbacks not getting run.

ggreenway · 2020-12-07T22:12:05Z

source/common/event/dispatcher_impl.cc

+    // callbacks execute. Callbacks added after this transfer will re-arm post_cb_ and will execute
+    // later in the event loop.
+    Thread::LockGuard lock(post_lock_);
+    if (post_callbacks_.empty()) {


I think this case can be deleted; the function behaves correctly without it AFAICT.

Done.

I was thinking of it as a micro optimization but didn't realize that most post_callbacks_ is usually non-empty when we get here.

ggreenway · 2020-12-07T22:13:07Z

/wait

Signed-off-by: Antonio Vicente <avd@google.com>

lizan

This changed the behavior that if a posted callback post another callback, it will be in next event cycle instead of the current one. Is that intended?

lizan · 2020-12-07T23:41:13Z

source/common/event/dispatcher_impl.cc

+  // It is important that the execution and deletion of the callback happen while post_lock_ is not
+  // held. Either the invocation or destructor of the callback can call post() on this dispatcher.
+  while (!callbacks.empty()) {
+    auto& callback = callbacks.front();


can be in oneline: callbacks.front().callback()?

I think the one line version would be:

callbacks.front()();

or

callbacks.front().operator();

yeah, either way, just style nit.

ggreenway · 2020-12-08T00:12:22Z

source/common/event/dispatcher_impl.cc

+  while (!callbacks.empty()) {
+    auto& callback = callbacks.front();
    callback();
+    // pop the front so that the destructor of the callback that just executed runs before the next


nit: capitalize Pop.

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente

This changed the behavior that if a posted callback post another callback, it will be in next event cycle instead of the current one. Is that intended?

Yes, there is a change in post loop behavior. The loop yields after the group of callbacks present is executed. Post callbacks added by the first set of post callbacks execute in the same event loop iteration (thanks to post_cb_->scheduleCallbackCurrentIteration()) but after other loop events have a chance to execute.

This change in behavior is desirable. I wanted to add a test to cover this behavior but I ran into an issue with ordering of post callbacks vs schedulable callbacks. I thought order was deterministic but apparently it isn't. The yield behavior will be testable as a consequence of #14293

antoniovicente · 2020-12-08T00:32:34Z

source/common/event/dispatcher_impl.cc

+  // It is important that the execution and deletion of the callback happen while post_lock_ is not
+  // held. Either the invocation or destructor of the callback can call post() on this dispatcher.
+  while (!callbacks.empty()) {
+    auto& callback = callbacks.front();


I think the one line version would be:

callbacks.front()();

or

callbacks.front().operator();

antoniovicente · 2020-12-08T00:48:24Z

source/common/event/dispatcher_impl.cc

+  while (!callbacks.empty()) {
+    auto& callback = callbacks.front();
    callback();
+    // pop the front so that the destructor of the callback that just executed runs before the next


…ocking Signed-off-by: Antonio Vicente <avd@google.com>

Signed-off-by: Antonio Vicente <avd@google.com>

test/common/event/dispatcher_impl_test.cc

ggreenway

LGTM, but please wait until @lizan responds before merging.

lizan

Just wanted to double-check the behavior change.

No strong preference on the nit, up to you.

lizan · 2020-12-08T23:43:50Z

source/common/event/dispatcher_impl.cc

+  // It is important that the execution and deletion of the callback happen while post_lock_ is not
+  // held. Either the invocation or destructor of the callback can call post() on this dispatcher.
+  while (!callbacks.empty()) {
+    auto& callback = callbacks.front();


yeah, either way, just style nit.

Signed-off-by: Antonio Vicente <avd@google.com>

* master: buffer: Optimize the layout of Slices in Buffer::OwnedImpl by removing subclassing and storing slice info directly in the SliceDeque (envoyproxy#14282) gRPC client to be used by ext_proc filter (envoyproxy#14283) http2: Add integration tests for PRIORITY frame flood mitigation for upstream servers (envoyproxy#14328) event: touch watchdog before execution of each post callback and before deferred deletion (envoyproxy#14339) stale: more allowed ops (envoyproxy#14345) stale: more changes (envoyproxy#14344) test: TODO fixup making enable_half_close private envoyproxy#14330) event: Reduce potential for lock contention while executing dispatcher post callbacks. (envoyproxy#14289) stale: fix config (envoyproxy#14337) metrics service sink: generalize the sink and grpc streamer for external use (envoyproxy#13919) wasm: update V8 to v8.8.278.8. (envoyproxy#14298) repo: switch to actions based stale bot (envoyproxy#14335) buffer: Use WatermarkFactory to create most WatermarkBuffer instances (envoyproxy#14256) Signed-off-by: Michael Puncel <mpuncel@squareup.com>

event: Reduce potential for lock contention while executing dispatche…

606f7af

…r post callbacks. Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente requested a review from jmarantz December 4, 2020 22:23

jmarantz self-assigned this Dec 4, 2020

jmarantz reviewed Dec 4, 2020

View reviewed changes

antoniovicente added 2 commits December 4, 2020 18:51

add comments and tests for low level details

c1451c8

Signed-off-by: Antonio Vicente <avd@google.com>

fix comment

24d6aab

Signed-off-by: Antonio Vicente <avd@google.com>

jmarantz previously approved these changes Dec 5, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into deferred_delete_l…

cff012b

…ocking Signed-off-by: Antonio Vicente <avd@google.com>

jmarantz assigned ggreenway Dec 5, 2020

ggreenway requested changes Dec 7, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 7, 2020

address review comments

e6b8e0c

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente dismissed jmarantz’s stale review via e6b8e0c December 7, 2020 22:22

repokitteh-read-only bot removed the waiting label Dec 7, 2020

lizan reviewed Dec 7, 2020

View reviewed changes

ggreenway reviewed Dec 8, 2020

View reviewed changes

Spelling

ca2292f

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente commented Dec 8, 2020

View reviewed changes

antoniovicente added 2 commits December 8, 2020 12:57

Merge remote-tracking branch 'upstream/master' into deferred_delete_l…

2268ca9

…ocking Signed-off-by: Antonio Vicente <avd@google.com>

add test for yield after each group of post callbacks.

53197d7

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente commented Dec 8, 2020

View reviewed changes

test/common/event/dispatcher_impl_test.cc Show resolved Hide resolved

ggreenway previously approved these changes Dec 8, 2020

View reviewed changes

antoniovicente assigned lizan Dec 8, 2020

antoniovicente requested a review from lizan December 8, 2020 23:14

lizan previously approved these changes Dec 8, 2020

View reviewed changes

style nit

ab34d08

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente dismissed stale reviews from lizan and ggreenway via ab34d08 December 9, 2020 00:01

lizan approved these changes Dec 9, 2020

View reviewed changes

antoniovicente merged commit bed0262 into envoyproxy:master Dec 9, 2020

Conversation

antoniovicente commented Dec 4, 2020

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Dec 7, 2020

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone