Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1467953002:
Implement MessagePumpMojo using WaitSet. (Closed)
|
Created:
5 years, 1 month ago by Anand Mistry (off Chromium) Modified:
4 years, 11 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chrome-apps-syd-reviews_chromium.org, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@mojo-waitset-implementation Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement MessagePumpMojo using WaitSet.
By using a wait set instead of WaitMany(), the message pump
no longer needs to do O(n) iterations over every waiting
handle on every iteration.
BUG=556865
Committed: https://crrev.com/6aebd413af88c78068be89cf174a2eafe5af3fe0
Cr-Commit-Position: refs/heads/master@{#367716}
Patch Set 1 #Patch Set 2 : Review reade. #
Total comments: 2
Patch Set 3 : Rebase. #
Total comments: 13
Patch Set 4 : Address review comments. #Patch Set 5 : Fix component builds. #
Total comments: 6
Patch Set 6 : Rebase, address comments, and auto format. #
Dependent Patchsets: Messages
Total messages: 23 (7 generated)
Description was changed from ========== Message pump implementaiton for WaitSet. NOT FOR SUBMIT... YET... ========== to ========== Message pump implementation for WaitSet. By using a wait set instead of WaitMany(), the message pump no longer needs to do O(n) iterations over every waiting handle on every iteration. BUG=556865 ==========
amistry@chromium.org changed reviewers: + jam@chromium.org, rockot@chromium.org, sky@chromium.org, yzshen@chromium.org
This is the last of a set of CLs to improve MessagePumpMojo performance. By how much you ask? Well, I ran page cycler against Mandoline before/after (https://googledrive.com/host/0B3QQn2XrgBSQQm54V01VZlRVVG8). Mostly an improvement in this "real-world" test (the statistically-significant 21% improvement in the verge is nice). Of course, microbenchmarks (mojo_common_perftests, currently broken) better reflect the improvement in scaling to the increasing number of waiting mojo handles.
Description was changed from ========== Message pump implementation for WaitSet. By using a wait set instead of WaitMany(), the message pump no longer needs to do O(n) iterations over every waiting handle on every iteration. BUG=556865 ========== to ========== Implement MessagePumpMojo using WaitSet. By using a wait set instead of WaitMany(), the message pump no longer needs to do O(n) iterations over every waiting handle on every iteration. BUG=556865 ==========
https://codereview.chromium.org/1467953002/diff/20001/mojo/message_pump/messa... File mojo/message_pump/message_pump_mojo.cc (right): https://codereview.chromium.org/1467953002/diff/20001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:18: #include "mojo/public/c/system/wait_set.h" It's a bit hard to review this patch without knowing waitset. Can you ping it once waitset lands?
https://codereview.chromium.org/1467953002/diff/20001/mojo/message_pump/messa... File mojo/message_pump/message_pump_mojo.cc (right): https://codereview.chromium.org/1467953002/diff/20001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:18: #include "mojo/public/c/system/wait_set.h" On 2015/12/08 18:26:14, sky wrote: > It's a bit hard to review this patch without knowing waitset. Can you ping it > once waitset lands? Will do. The API is defined in https://codereview.chromium.org/1429553002/, for anyone that casually comes across this CL.
PTAL. The API definition as well as old EDK implementation are submitted. The new EDK implementation is in the CQ. This is ready to go.
https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... File mojo/message_pump/message_pump_mojo.cc (right): https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:108: CHECK(result == MOJO_RESULT_OK || result == MOJO_RESULT_INVALID_ARGUMENT); Can you add test coverage of the add with closed pipe? I don't think we have that as previously we did not allow adding closed handles. https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:197: const MojoResult wait_result = Wait(wait_set_handle_.get(), How come you don't always call wait? https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:212: const uint32_t kMaxServiced = 8; This function is rather lengthy. How about breaking it up? Maybe move this whole block into its own function? https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:236: // Skip handles that have been removed. Under what conditions could this happen? I'm wondering why it isn't a DCHECK? https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:272: ReadMessageRaw(read_handle_.get(), NULL, NULL, NULL, NULL, nullptr on these? https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:330: if (handlers_.find(handle) == handlers_.end()) Under what conditions would this hit?
I've already split the handle change into https://codereview.chromium.org/1526493002/ https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... File mojo/message_pump/message_pump_mojo.cc (right): https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:108: CHECK(result == MOJO_RESULT_OK || result == MOJO_RESULT_INVALID_ARGUMENT); On 2015/12/11 17:49:01, sky OOO until 28th wrote: > Can you add test coverage of the add with closed pipe? I don't think we have > that as previously we did not allow adding closed handles. Hm. Here's a question for you. If you add a closed handle, should it result in the error handler being called in the next loop iteration, or just be ignored and assume the caller will remove it. Currently, the latter happens. But maybe this function should be more proactive. https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:197: const MojoResult wait_result = Wait(wait_set_handle_.get(), On 2015/12/11 17:49:01, sky OOO until 28th wrote: > How come you don't always call wait? Added a comment to address. https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:212: const uint32_t kMaxServiced = 8; On 2015/12/11 17:49:01, sky OOO until 28th wrote: > This function is rather lengthy. How about breaking it up? Maybe move this whole > block into its own function? Done. https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:236: // Skip handles that have been removed. On 2015/12/11 17:49:01, sky OOO until 28th wrote: > Under what conditions could this happen? I'm wondering why it isn't a DCHECK? Added comment to explain why this is possible. It's a consequence of https://codereview.chromium.org/1475983004, and that handles can be closed before they are removed from the wait. https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:272: ReadMessageRaw(read_handle_.get(), NULL, NULL, NULL, NULL, On 2015/12/11 17:49:01, sky OOO until 28th wrote: > nullptr on these? Done. https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:330: if (handlers_.find(handle) == handlers_.end()) On 2015/12/11 17:49:01, sky OOO until 28th wrote: > Under what conditions would this hit? It did in a previous iteration, but not any more. Put the CHECK back.
Hi, Anand. If Scott thinks the CL is ready, please feel free to land it without my LG. But if you think I should look at it, please let me know and I will do it before I take vacations (starting from Wed). Thanks!
https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... File mojo/message_pump/message_pump_mojo.cc (right): https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:108: CHECK(result == MOJO_RESULT_OK || result == MOJO_RESULT_INVALID_ARGUMENT); On 2015/12/14 03:16:20, Anand Mistry wrote: > On 2015/12/11 17:49:01, sky OOO until 28th wrote: > > Can you add test coverage of the add with closed pipe? I don't think we have > > that as previously we did not allow adding closed handles. > > Hm. Here's a question for you. If you add a closed handle, should it result in > the error handler being called in the next loop iteration, or just be ignored > and assume the caller will remove it. Currently, the latter happens. But maybe > this function should be more proactive. Is there a reason you don't want to notify immediately?
On 2015/12/28 17:29:44, sky wrote: > https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... > File mojo/message_pump/message_pump_mojo.cc (right): > > https://codereview.chromium.org/1467953002/diff/40001/mojo/message_pump/messa... > mojo/message_pump/message_pump_mojo.cc:108: CHECK(result == MOJO_RESULT_OK || > result == MOJO_RESULT_INVALID_ARGUMENT); > On 2015/12/14 03:16:20, Anand Mistry wrote: > > On 2015/12/11 17:49:01, sky OOO until 28th wrote: > > > Can you add test coverage of the add with closed pipe? I don't think we have > > > that as previously we did not allow adding closed handles. > > > > Hm. Here's a question for you. If you add a closed handle, should it result in > > the error handler being called in the next loop iteration, or just be ignored > > and assume the caller will remove it. Currently, the latter happens. But maybe > > this function should be more proactive. > > Is there a reason you don't want to notify immediately? I think the reentrency might be problematic. Having AddHandler() run OnHandleError(), which can then add or remove handles, smells bad. Also, I have a (questionable) follow-up CL that lets HandleWatcher avoid thread hoping when start/stopping, and the reentrency here will make the necessary locking more complex.
Ok, LGTM
lgtm https://codereview.chromium.org/1467953002/diff/80001/mojo/message_pump/messa... File mojo/message_pump/message_pump_mojo.cc (right): https://codereview.chromium.org/1467953002/diff/80001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:60: result = MojoCreateWaitSet(&handle); general no-op comment: We should consider adding a C++ API for WaitSet. https://codereview.chromium.org/1467953002/diff/80001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:234: uint32_t count = kMaxServiced; nit: how about num_ready_handles or something more descriptive
LGTM https://codereview.chromium.org/1467953002/diff/80001/mojo/message_pump/messa... File mojo/message_pump/message_pump_mojo.h (right): https://codereview.chromium.org/1467953002/diff/80001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.h:104: // Removes the given invalid handle. This is called if MojoWaitMany finds an nit: please update the comment.
I'm back, and will be submitting this asap. Thanks for the reviews. https://codereview.chromium.org/1467953002/diff/80001/mojo/message_pump/messa... File mojo/message_pump/message_pump_mojo.cc (right): https://codereview.chromium.org/1467953002/diff/80001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:60: result = MojoCreateWaitSet(&handle); On 2016/01/05 17:02:29, Ken Rockot wrote: > general no-op comment: We should consider adding a C++ API for WaitSet. Ack. It would be nice, but since this is the only user, I consider it a lower priority. https://codereview.chromium.org/1467953002/diff/80001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.cc:234: uint32_t count = kMaxServiced; On 2016/01/05 17:02:29, Ken Rockot wrote: > nit: how about num_ready_handles or something more descriptive Done. https://codereview.chromium.org/1467953002/diff/80001/mojo/message_pump/messa... File mojo/message_pump/message_pump_mojo.h (right): https://codereview.chromium.org/1467953002/diff/80001/mojo/message_pump/messa... mojo/message_pump/message_pump_mojo.h:104: // Removes the given invalid handle. This is called if MojoWaitMany finds an On 2016/01/05 17:41:05, yzshen1 wrote: > nit: please update the comment. Done.
The CQ bit was checked by amistry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, sky@chromium.org, yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/1467953002/#ps100001 (title: "Rebase, address comments, and auto format.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1467953002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1467953002/100001
Message was sent while issue was closed.
Description was changed from ========== Implement MessagePumpMojo using WaitSet. By using a wait set instead of WaitMany(), the message pump no longer needs to do O(n) iterations over every waiting handle on every iteration. BUG=556865 ========== to ========== Implement MessagePumpMojo using WaitSet. By using a wait set instead of WaitMany(), the message pump no longer needs to do O(n) iterations over every waiting handle on every iteration. BUG=556865 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Implement MessagePumpMojo using WaitSet. By using a wait set instead of WaitMany(), the message pump no longer needs to do O(n) iterations over every waiting handle on every iteration. BUG=556865 ========== to ========== Implement MessagePumpMojo using WaitSet. By using a wait set instead of WaitMany(), the message pump no longer needs to do O(n) iterations over every waiting handle on every iteration. BUG=556865 Committed: https://crrev.com/6aebd413af88c78068be89cf174a2eafe5af3fe0 Cr-Commit-Position: refs/heads/master@{#367716} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6aebd413af88c78068be89cf174a2eafe5af3fe0 Cr-Commit-Position: refs/heads/master@{#367716} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
