Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2162143002:
Don't use PostTask queueing between compositor and main thread. (Closed)
|
Created:
4 years, 5 months ago by dtapuska Modified:
4 years, 4 months ago Reviewers:
tdresser CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, dtapuska+chromiumwatch_chromium.org, dglazkov+blink, darin-cc_chromium.org, blink-reviews, blink-reviews-api_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't use PostTask queueing between compositor and main thread.
Instead put the events in the main thread event queue where they can
be coalesced. This improves the latency on mouse move events ~5-7X
On Linux loading https://jsfiddle.net/va5vpxLz/6/
and getting ~250 mouse samples
Stable (RAF)
53.579747191011386
Stable (setTimeout)
27.226215139442306
With Change (RAF)
7.244861660078827
With Change (setTimeout)
5.327854330708663
BUG=624012, 599152
Committed: https://crrev.com/4d87f4e9da90c5fdc94ad48cd6b5c888ef8bd896
Cr-Commit-Position: refs/heads/master@{#410487}
Patch Set 1 #Patch Set 2 : Fix android build and unit tests #Patch Set 3 : Don't ack mouse move right away send them unthrottled #
Total comments: 29
Patch Set 4 : Rebase #
Total comments: 10
Patch Set 5 : Add browser test, fix comments from #4 #
Total comments: 23
Patch Set 6 : Address issues with test and fix dchecks in debug mode #
Total comments: 2
Patch Set 7 : Fix fairness of post tasks it was causing some tests to fail #Messages
Total messages: 48 (31 generated)
The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dtapuska@chromium.org changed reviewers: + tdresser@chromium.org
Tim please take a look at this
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Some nits and some questions. https://codereview.chromium.org/2162143002/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/2162143002/diff/40001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:191: mouse_move_queue_.push_back(mouse_event); Don't we still only need the most recent mousemove? Is a queue needed? https://codereview.chromium.org/2162143002/diff/40001/content/common/input/ev... File content/common/input/event_with_latency_info.cc (right): https://codereview.chromium.org/2162143002/diff/40001/content/common/input/ev... content/common/input/event_with_latency_info.cc:281: // for Telemetry latency tests, since it will represent the longest While you're here, we shouldn't really say "for Telemetry latency tests", since it's used for other things as well. https://codereview.chromium.org/2162143002/diff/40001/content/common/input/ev... File content/common/input/event_with_latency_info.h (right): https://codereview.chromium.org/2162143002/diff/40001/content/common/input/ev... content/common/input/event_with_latency_info.h:56: mutable ui::LatencyInfo latency_; I don't feel great about this being mutable. It's needed for coalescing, right? Could we just make the event being coalesced non-const? https://codereview.chromium.org/2162143002/diff/40001/content/common/input/we... File content/common/input/web_input_event_queue.h (right): https://codereview.chromium.org/2162143002/diff/40001/content/common/input/we... content/common/input/web_input_event_queue.h:13: // WebInputEventQueue is a coalescing queue. Could the changes in this file be made separately from the other changes? https://codereview.chromium.org/2162143002/diff/40001/content/common/input/we... content/common/input/web_input_event_queue.h:17: // send notification This is a somewhat odd pseudo-pseudo-code. I'd prefer it be either more or less pseudo-code. In particular, ending some lines with ; and others without is kinda weird. Maybe the last line should be if (send_notification) { send T; } https://codereview.chromium.org/2162143002/diff/40001/content/common/input/we... content/common/input/web_input_event_queue.h:24: // queue.set_state(WebInputEventQueueState::ITEM_NOT_PENDING); Update comment. https://codereview.chromium.org/2162143002/diff/40001/content/common/input/we... content/common/input/web_input_event_queue.h:36: last_event_iter != queue_.rend(); ++last_event_iter) { Range based for? https://codereview.chromium.org/2162143002/diff/40001/content/common/input/we... content/common/input/web_input_event_queue.h:41: if ((*last_event_iter)->CanCoalesceWith(*event.get())) { Could we get weird out of order coalescing happening with this logic? For example, if there were two separate scroll gestures in the queue, could we coalesce into the wrong gesture? https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... File content/renderer/input/input_event_filter.cc (right): https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... content/renderer/input/input_event_filter.cc:118: queue->EventHandled(type, ack_result); Does the queue really need to be refptr? https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... content/renderer/input/input_event_filter.cc:245: TRACE_EVENT_SCOPE_THREAD); This rename could potentially break some telemetry stuff. (Go ahead and land it, but be aware it's a possibility, or do some searching to see if any telemetry measurements use this data) https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... File content/renderer/input/input_event_filter_unittest.cc (right): https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... content/renderer/input/input_event_filter_unittest.cc:217: { Should we get rid of the {}? https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... File content/renderer/input/main_thread_event_queue.cc (right): https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:117: for (const auto id : in_flight_event_->eventsToAck()) I don't think we should use auto here. https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... File content/renderer/input/main_thread_event_queue.h (right): https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... content/renderer/input/main_thread_event_queue.h:36: std::deque<uint32_t> eventsToAck_; Make it clear this is touch only. https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... content/renderer/input/main_thread_event_queue.h:57: // and one mouse wheel) for events that need to be queued between Update comment. https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... content/renderer/input/main_thread_event_queue.h:129: bool is_flinging_; Do we still need is_flinging? https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... File content/renderer/input/render_widget_input_handler.cc (right): https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... content/renderer/input/render_widget_input_handler.cc:423: // Note that we can't use handling_event_type_ here since it will be overriden While you're here, could you fix overriden -> overridden?
https://codereview.chromium.org/2162143002/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/2162143002/diff/40001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:191: mouse_move_queue_.push_back(mouse_event); On 2016/07/20 20:52:27, tdresser wrote: > Don't we still only need the most recent mousemove? Is a queue needed? Unfortunately with the latency info we need to have the original event. We can see if we can clean this code up but from my initial look is that it needs the events to query a few fields like the latency input coordinates it adds to the latency info object. https://codereview.chromium.org/2162143002/diff/40001/content/common/input/ev... File content/common/input/event_with_latency_info.cc (right): https://codereview.chromium.org/2162143002/diff/40001/content/common/input/ev... content/common/input/event_with_latency_info.cc:281: // for Telemetry latency tests, since it will represent the longest On 2016/07/20 20:52:27, tdresser wrote: > While you're here, we shouldn't really say "for Telemetry latency tests", since > it's used for other things as well. Done. https://codereview.chromium.org/2162143002/diff/40001/content/common/input/we... File content/common/input/web_input_event_queue.h (right): https://codereview.chromium.org/2162143002/diff/40001/content/common/input/we... content/common/input/web_input_event_queue.h:13: // WebInputEventQueue is a coalescing queue. On 2016/07/20 20:52:27, tdresser wrote: > Could the changes in this file be made separately from the other changes? Done. https://codereview.chromium.org/2162143002/diff/40001/content/common/input/we... content/common/input/web_input_event_queue.h:17: // send notification On 2016/07/20 20:52:27, tdresser wrote: > This is a somewhat odd pseudo-pseudo-code. > > I'd prefer it be either more or less pseudo-code. > > In particular, ending some lines with ; and others without is kinda weird. > > Maybe the last line should be > > if (send_notification) { > send T; > } Done. https://codereview.chromium.org/2162143002/diff/40001/content/common/input/we... content/common/input/web_input_event_queue.h:36: last_event_iter != queue_.rend(); ++last_event_iter) { On 2016/07/20 20:52:27, tdresser wrote: > Range based for? It's a reverse iteration; so you can't do a reversed range based for in C++11 https://codereview.chromium.org/2162143002/diff/40001/content/common/input/we... content/common/input/web_input_event_queue.h:41: if ((*last_event_iter)->CanCoalesceWith(*event.get())) { On 2016/07/20 20:52:27, tdresser wrote: > Could we get weird out of order coalescing happening with this logic? > > For example, if there were two separate scroll gestures in the queue, could we > coalesce into the wrong gesture? No because it should stop at the most recent gesture sequence. https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... File content/renderer/input/input_event_filter.cc (right): https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... content/renderer/input/input_event_filter.cc:118: queue->EventHandled(type, ack_result); On 2016/07/20 20:52:27, tdresser wrote: > Does the queue really need to be refptr? yes for thread safety. https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... content/renderer/input/input_event_filter.cc:245: TRACE_EVENT_SCOPE_THREAD); On 2016/07/20 20:52:27, tdresser wrote: > This rename could potentially break some telemetry stuff. (Go ahead and land it, > but be aware it's a possibility, or do some searching to see if any telemetry > measurements use this data) I grepped catapult and didn't see any use of the string. https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... File content/renderer/input/input_event_filter_unittest.cc (right): https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... content/renderer/input/input_event_filter_unittest.cc:217: { On 2016/07/20 20:52:27, tdresser wrote: > Should we get rid of the {}? I was trying to scope the local variables that I was testing. This was consistent in the test case that this is done. https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... File content/renderer/input/main_thread_event_queue.cc (right): https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:117: for (const auto id : in_flight_event_->eventsToAck()) On 2016/07/20 20:52:27, tdresser wrote: > I don't think we should use auto here. Is there a reason why? https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... File content/renderer/input/main_thread_event_queue.h (right): https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... content/renderer/input/main_thread_event_queue.h:36: std::deque<uint32_t> eventsToAck_; On 2016/07/20 20:52:27, tdresser wrote: > Make it clear this is touch only. Acknowledged. https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... content/renderer/input/main_thread_event_queue.h:129: bool is_flinging_; On 2016/07/20 20:52:27, tdresser wrote: > Do we still need is_flinging? yes sorry that was missing code.
The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I'd like to see an integration test for this change – we want to make sure that we keep the property that the main thread pulls the most recent event. Think we could do an integration test of some variety here? A browsertest perhaps? https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... File content/renderer/input/main_thread_event_queue.cc (right): https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:117: for (const auto id : in_flight_event_->eventsToAck()) On 2016/07/27 05:29:00, dtapuska wrote: > On 2016/07/20 20:52:27, tdresser wrote: > > I don't think we should use auto here. > > Is there a reason why? From the style guide: "auto is permitted ... when it increases readability" That's pretty ambiguous, but in this case, I don't feel it increases readability. I'd say readability is about equal either way, and auto is only to be used when readability is improved. https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... File content/renderer/input/main_thread_event_queue.cc (right): https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:73: static_cast<blink::WebTouchEvent&>(cloned_event->event()) Having this cast twice is a bit unfortunate. This might be easier to read as: if (is_touch) { blink::WebTouchWheelEvent touch_event(); touch_event.dispatchedDuringFling = is_flinging; if (non_blocking) touch_event.dispatchType = blink::WebInputEvent::ListenersNonBlockingPassive; } else if (is_wheel) { blink::WebMouseWheelEvent wheel_event(); if (non_blocking) wheel_event.dispatchType = blink::WebInputEvent::ListenersNonBlockingPassive; } https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... File content/renderer/input/main_thread_event_queue.h (right): https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.h:94: // <-------(ACK)------ This comment needs updating, doesn't it? https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.h:136: base::Lock event_queue_mutex_; Looks like event_queue_lock_ would be better in line with other lock naming. https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... File content/renderer/input/main_thread_event_queue_unittest.cc (right): https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue_unittest.cc:73: ASSERT_FALSE(main_task_runner_->HasPendingTask()); Why are we using ASSERT instead of EXPECT throughout this file? We should probably be using EXPECT in most places. https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue_unittest.cc:100: memcmp(&coalesced_event, last_wheel_event, coalesced_event.size) == 0); Do we not have a usable equality check? We could define an equality operator in this file, and then use EXPECT_EQ.
On 2016/07/27 13:51:51, tdresser wrote: > I'd like to see an integration test for this change – we want to make sure that > we keep the property that the main thread pulls the most recent event. > > Think we could do an integration test of some variety here? A browsertest > perhaps? > > https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... > File content/renderer/input/main_thread_event_queue.cc (right): > > https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... > content/renderer/input/main_thread_event_queue.cc:117: for (const auto id : > in_flight_event_->eventsToAck()) > On 2016/07/27 05:29:00, dtapuska wrote: > > On 2016/07/20 20:52:27, tdresser wrote: > > > I don't think we should use auto here. > > > > Is there a reason why? > > From the style guide: > "auto is permitted ... when it increases readability" > > That's pretty ambiguous, but in this case, I don't feel it increases > readability. I'd say readability is about equal either way, and auto is only to > be used when readability is improved. > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > File content/renderer/input/main_thread_event_queue.cc (right): > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > content/renderer/input/main_thread_event_queue.cc:73: > static_cast<blink::WebTouchEvent&>(cloned_event->event()) > Having this cast twice is a bit unfortunate. > > This might be easier to read as: > > if (is_touch) { > blink::WebTouchWheelEvent touch_event(); > touch_event.dispatchedDuringFling = is_flinging; > if (non_blocking) > touch_event.dispatchType = > blink::WebInputEvent::ListenersNonBlockingPassive; > } else if (is_wheel) { > blink::WebMouseWheelEvent wheel_event(); > if (non_blocking) > wheel_event.dispatchType = > blink::WebInputEvent::ListenersNonBlockingPassive; > } > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > File content/renderer/input/main_thread_event_queue.h (right): > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > content/renderer/input/main_thread_event_queue.h:94: // <-------(ACK)------ > This comment needs updating, doesn't it? > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > content/renderer/input/main_thread_event_queue.h:136: base::Lock > event_queue_mutex_; > Looks like event_queue_lock_ would be better in line with other lock naming. > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > File content/renderer/input/main_thread_event_queue_unittest.cc (right): > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > content/renderer/input/main_thread_event_queue_unittest.cc:73: > ASSERT_FALSE(main_task_runner_->HasPendingTask()); > Why are we using ASSERT instead of EXPECT throughout this file? We should > probably be using EXPECT in most places. > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > content/renderer/input/main_thread_event_queue_unittest.cc:100: > memcmp(&coalesced_event, last_wheel_event, coalesced_event.size) == 0); > Do we not have a usable equality check? We could define an equality operator in > this file, and then use EXPECT_EQ.
On 2016/07/27 14:09:31, dtapuska wrote: > On 2016/07/27 13:51:51, tdresser wrote: > > I'd like to see an integration test for this change – we want to make sure > that > > we keep the property that the main thread pulls the most recent event. > > > > Think we could do an integration test of some variety here? A browsertest > > perhaps? > > > > > https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... > > File content/renderer/input/main_thread_event_queue.cc (right): > > > > > https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... > > content/renderer/input/main_thread_event_queue.cc:117: for (const auto id : > > in_flight_event_->eventsToAck()) > > On 2016/07/27 05:29:00, dtapuska wrote: > > > On 2016/07/20 20:52:27, tdresser wrote: > > > > I don't think we should use auto here. > > > > > > Is there a reason why? > > > > From the style guide: > > "auto is permitted ... when it increases readability" > > > > That's pretty ambiguous, but in this case, I don't feel it increases > > readability. I'd say readability is about equal either way, and auto is only > to > > be used when readability is improved. > > > > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > > File content/renderer/input/main_thread_event_queue.cc (right): > > > > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > > content/renderer/input/main_thread_event_queue.cc:73: > > static_cast<blink::WebTouchEvent&>(cloned_event->event()) > > Having this cast twice is a bit unfortunate. > > > > This might be easier to read as: > > > > if (is_touch) { > > blink::WebTouchWheelEvent touch_event(); > > touch_event.dispatchedDuringFling = is_flinging; > > if (non_blocking) > > touch_event.dispatchType = > > blink::WebInputEvent::ListenersNonBlockingPassive; > > } else if (is_wheel) { > > blink::WebMouseWheelEvent wheel_event(); > > if (non_blocking) > > wheel_event.dispatchType = > > blink::WebInputEvent::ListenersNonBlockingPassive; > > } > > > > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > > File content/renderer/input/main_thread_event_queue.h (right): > > > > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > > content/renderer/input/main_thread_event_queue.h:94: // <-------(ACK)------ > > This comment needs updating, doesn't it? > > > > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > > content/renderer/input/main_thread_event_queue.h:136: base::Lock > > event_queue_mutex_; > > Looks like event_queue_lock_ would be better in line with other lock naming. > > > > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > > File content/renderer/input/main_thread_event_queue_unittest.cc (right): > > > > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > > content/renderer/input/main_thread_event_queue_unittest.cc:73: > > ASSERT_FALSE(main_task_runner_->HasPendingTask()); > > Why are we using ASSERT instead of EXPECT throughout this file? We should > > probably be using EXPECT in most places. > > > > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > > content/renderer/input/main_thread_event_queue_unittest.cc:100: > > memcmp(&coalesced_event, last_wheel_event, coalesced_event.size) == 0); > > Do we not have a usable equality check? We could define an equality operator > in > > this file, and then use EXPECT_EQ. > Think we could do an integration test of some variety here? A browsertest perhaps? So something like make the main thread janking. Send two or three touch moves and ensure the main thread only reports the last event? Likewise with mouse move?
On 2016/07/27 14:10:50, dtapuska wrote: > On 2016/07/27 14:09:31, dtapuska wrote: > > On 2016/07/27 13:51:51, tdresser wrote: > > > I'd like to see an integration test for this change – we want to make sure > > that > > > we keep the property that the main thread pulls the most recent event. > > > > > > Think we could do an integration test of some variety here? A browsertest > > > perhaps? > > > > > > > > > https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... > > > File content/renderer/input/main_thread_event_queue.cc (right): > > > > > > > > > https://codereview.chromium.org/2162143002/diff/40001/content/renderer/input/... > > > content/renderer/input/main_thread_event_queue.cc:117: for (const auto id : > > > in_flight_event_->eventsToAck()) > > > On 2016/07/27 05:29:00, dtapuska wrote: > > > > On 2016/07/20 20:52:27, tdresser wrote: > > > > > I don't think we should use auto here. > > > > > > > > Is there a reason why? > > > > > > From the style guide: > > > "auto is permitted ... when it increases readability" > > > > > > That's pretty ambiguous, but in this case, I don't feel it increases > > > readability. I'd say readability is about equal either way, and auto is only > > to > > > be used when readability is improved. > > > > > > > > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > > > File content/renderer/input/main_thread_event_queue.cc (right): > > > > > > > > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > > > content/renderer/input/main_thread_event_queue.cc:73: > > > static_cast<blink::WebTouchEvent&>(cloned_event->event()) > > > Having this cast twice is a bit unfortunate. > > > > > > This might be easier to read as: > > > > > > if (is_touch) { > > > blink::WebTouchWheelEvent touch_event(); > > > touch_event.dispatchedDuringFling = is_flinging; > > > if (non_blocking) > > > touch_event.dispatchType = > > > blink::WebInputEvent::ListenersNonBlockingPassive; > > > } else if (is_wheel) { > > > blink::WebMouseWheelEvent wheel_event(); > > > if (non_blocking) > > > wheel_event.dispatchType = > > > blink::WebInputEvent::ListenersNonBlockingPassive; > > > } > > > > > > > > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > > > File content/renderer/input/main_thread_event_queue.h (right): > > > > > > > > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > > > content/renderer/input/main_thread_event_queue.h:94: // > <-------(ACK)------ > > > This comment needs updating, doesn't it? > > > > > > > > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > > > content/renderer/input/main_thread_event_queue.h:136: base::Lock > > > event_queue_mutex_; > > > Looks like event_queue_lock_ would be better in line with other lock naming. > > > > > > > > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > > > File content/renderer/input/main_thread_event_queue_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > > > content/renderer/input/main_thread_event_queue_unittest.cc:73: > > > ASSERT_FALSE(main_task_runner_->HasPendingTask()); > > > Why are we using ASSERT instead of EXPECT throughout this file? We should > > > probably be using EXPECT in most places. > > > > > > > > > https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... > > > content/renderer/input/main_thread_event_queue_unittest.cc:100: > > > memcmp(&coalesced_event, last_wheel_event, coalesced_event.size) == 0); > > > Do we not have a usable equality check? We could define an equality operator > > in > > > this file, and then use EXPECT_EQ. > > > Think we could do an integration test of some variety here? A browsertest > perhaps? > > So something like make the main thread janking. Send two or three touch moves > and ensure the main thread only reports the last event? > Likewise with mouse move? Exactly.
The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... File content/renderer/input/main_thread_event_queue.cc (right): https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:73: static_cast<blink::WebTouchEvent&>(cloned_event->event()) On 2016/07/27 13:51:51, tdresser wrote: > Having this cast twice is a bit unfortunate. > > This might be easier to read as: > > if (is_touch) { > blink::WebTouchWheelEvent touch_event(); > touch_event.dispatchedDuringFling = is_flinging; > if (non_blocking) > touch_event.dispatchType = > blink::WebInputEvent::ListenersNonBlockingPassive; > } else if (is_wheel) { > blink::WebMouseWheelEvent wheel_event(); > if (non_blocking) > wheel_event.dispatchType = > blink::WebInputEvent::ListenersNonBlockingPassive; > } Done. https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... File content/renderer/input/main_thread_event_queue.h (right): https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.h:94: // <-------(ACK)------ On 2016/07/27 13:51:51, tdresser wrote: > This comment needs updating, doesn't it? Done. https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.h:136: base::Lock event_queue_mutex_; On 2016/07/27 13:51:51, tdresser wrote: > Looks like event_queue_lock_ would be better in line with other lock naming. Done. https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... File content/renderer/input/main_thread_event_queue_unittest.cc (right): https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue_unittest.cc:73: ASSERT_FALSE(main_task_runner_->HasPendingTask()); On 2016/07/27 13:51:51, tdresser wrote: > Why are we using ASSERT instead of EXPECT throughout this file? We should > probably be using EXPECT in most places. Done. https://codereview.chromium.org/2162143002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue_unittest.cc:100: memcmp(&coalesced_event, last_wheel_event, coalesced_event.size) == 0); On 2016/07/27 13:51:51, tdresser wrote: > Do we not have a usable equality check? We could define an equality operator in > this file, and then use EXPECT_EQ. Done
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2162143002/diff/80001/content/browser/rendere... File content/browser/renderer_host/input/main_thread_event_queue_browsertest.cc (right): https://codereview.chromium.org/2162143002/diff/80001/content/browser/rendere... content/browser/renderer_host/input/main_thread_event_queue_browsertest.cc:63: " document.addEventListener('mousemove', mouseMoveCounter);" I'd be tempted just to make these method anonymous, or give them more generic names like "onMouseMove". Right now, the "counter" methods are doing more than just count, which is a bit confusing. https://codereview.chromium.org/2162143002/diff/80001/content/browser/rendere... content/browser/renderer_host/input/main_thread_event_queue_browsertest.cc:114: blink::WebPointerProperties::ButtonLeft); Why not just call jank() explicitly here? https://codereview.chromium.org/2162143002/diff/80001/content/browser/rendere... content/browser/renderer_host/input/main_thread_event_queue_browsertest.cc:124: // Runs until we get the InputMsgAck callback Missing . https://codereview.chromium.org/2162143002/diff/80001/content/browser/rendere... content/browser/renderer_host/input/main_thread_event_queue_browsertest.cc:129: ExecuteScriptAndExtractInt("window.mouseMoveCount")) <= 0) { I'd find this easier to read as: int mouse_move_count = 0; while (move_move_count <= 0) mouse_move_count = ExecuteScriptAndExtractInt("window.mouseMoveCount") This is going to busy loop though, right? Is that actually the right thing to do? https://codereview.chromium.org/2162143002/diff/80001/content/browser/rendere... content/browser/renderer_host/input/main_thread_event_queue_browsertest.cc:160: // Runs until we get the InputMsgAck callback Missing . https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... File content/renderer/input/main_thread_event_queue.cc (right): https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:106: dispatch_type == DISPATCH_TYPE_BLOCKING) I prefer {} when the condition or the body is multiline, though this isn't technically required by the style guide. Leave it if you prefer (but adding {} is definitely superior :) ) https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:137: void MainThreadEventQueue::SendEventNotificationToMainThread() { I'm a bit confused here. SendEventNotificationToMainThread posts a task to PopEventOnMainThread, which then may call SendEventNotificationToMainThread. Which threads do these methods run on? Are we posting PopEventOnMainThread onto the main thread from the main thread? https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... File content/renderer/input/main_thread_event_queue.h (right): https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... content/renderer/input/main_thread_event_queue.h:62: // by a lock primarily where events are enqued by the compositor thread Is the "primarily" required here? When is this not the case? https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... content/renderer/input/main_thread_event_queue.h:62: // by a lock primarily where events are enqued by the compositor thread enqued -> enqueued https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... content/renderer/input/main_thread_event_queue.h:63: // and dequed by the main thread. dequed -> dequeued. https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... content/renderer/input/main_thread_event_queue.h:93: // <-------(ACK)------ Thanks, this looks great.
The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2162143002/diff/80001/content/browser/rendere... File content/browser/renderer_host/input/main_thread_event_queue_browsertest.cc (right): https://codereview.chromium.org/2162143002/diff/80001/content/browser/rendere... content/browser/renderer_host/input/main_thread_event_queue_browsertest.cc:63: " document.addEventListener('mousemove', mouseMoveCounter);" On 2016/08/04 18:02:22, tdresser wrote: > I'd be tempted just to make these method anonymous, or give them more generic > names like "onMouseMove". > > Right now, the "counter" methods are doing more than just count, which is a bit > confusing. Done. https://codereview.chromium.org/2162143002/diff/80001/content/browser/rendere... content/browser/renderer_host/input/main_thread_event_queue_browsertest.cc:114: blink::WebPointerProperties::ButtonLeft); On 2016/08/04 18:02:21, tdresser wrote: > Why not just call jank() explicitly here? Because ExecuteScript is synchronous and I want to simulate real behaviour. I've added a comment. https://codereview.chromium.org/2162143002/diff/80001/content/browser/rendere... content/browser/renderer_host/input/main_thread_event_queue_browsertest.cc:124: // Runs until we get the InputMsgAck callback On 2016/08/04 18:02:22, tdresser wrote: > Missing . Done. https://codereview.chromium.org/2162143002/diff/80001/content/browser/rendere... content/browser/renderer_host/input/main_thread_event_queue_browsertest.cc:129: ExecuteScriptAndExtractInt("window.mouseMoveCount")) <= 0) { On 2016/08/04 18:02:22, tdresser wrote: > I'd find this easier to read as: > > int mouse_move_count = 0; > while (move_move_count <= 0) > mouse_move_count = ExecuteScriptAndExtractInt("window.mouseMoveCount") > > This is going to busy loop though, right? > Is that actually the right thing to do? Done. It isn't a real busy loop as ExecuteScriptAndExtractInt puts something on the main thread and waits for it. Most of the time this ends in one iteration. https://codereview.chromium.org/2162143002/diff/80001/content/browser/rendere... content/browser/renderer_host/input/main_thread_event_queue_browsertest.cc:160: // Runs until we get the InputMsgAck callback On 2016/08/04 18:02:22, tdresser wrote: > Missing . Done. https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... File content/renderer/input/main_thread_event_queue.cc (right): https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:106: dispatch_type == DISPATCH_TYPE_BLOCKING) On 2016/08/04 18:02:22, tdresser wrote: > I prefer {} when the condition or the body is multiline, though this isn't > technically required by the style guide. > > Leave it if you prefer (but adding {} is definitely superior :) ) Done. https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:137: void MainThreadEventQueue::SendEventNotificationToMainThread() { On 2016/08/04 18:02:22, tdresser wrote: > I'm a bit confused here. > > SendEventNotificationToMainThread posts a task to PopEventOnMainThread, which > then may call SendEventNotificationToMainThread. > > Which threads do these methods run on? > > Are we posting PopEventOnMainThread onto the main thread from the main thread? In the currently (ToT) event processing we only process one event per message loop iteration. This design is to maintain that behavior. This runs on both threads; when an event is enqueued it will run on the cc thread and when we have pop and event from the main thread it will run if there is remaining work. I do change some of this for rAF aligned input processing up to a number of events per iteration. https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... File content/renderer/input/main_thread_event_queue.h (right): https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... content/renderer/input/main_thread_event_queue.h:62: // by a lock primarily where events are enqued by the compositor thread On 2016/08/04 18:02:22, tdresser wrote: > enqued -> enqueued Done. https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... content/renderer/input/main_thread_event_queue.h:62: // by a lock primarily where events are enqued by the compositor thread On 2016/08/04 18:02:22, tdresser wrote: > enqued -> enqueued Done. https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... content/renderer/input/main_thread_event_queue.h:63: // and dequed by the main thread. On 2016/08/04 18:02:22, tdresser wrote: > dequed -> dequeued. Done. https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... content/renderer/input/main_thread_event_queue.h:93: // <-------(ACK)------ On 2016/08/04 18:02:22, tdresser wrote: > Thanks, this looks great. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
LGTM https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... File content/renderer/input/main_thread_event_queue.cc (right): https://codereview.chromium.org/2162143002/diff/80001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:137: void MainThreadEventQueue::SendEventNotificationToMainThread() { On 2016/08/08 15:34:03, dtapuska wrote: > On 2016/08/04 18:02:22, tdresser wrote: > > I'm a bit confused here. > > > > SendEventNotificationToMainThread posts a task to PopEventOnMainThread, which > > then may call SendEventNotificationToMainThread. > > > > Which threads do these methods run on? > > > > Are we posting PopEventOnMainThread onto the main thread from the main thread? > > In the currently (ToT) event processing we only process one event per message > loop iteration. > > This design is to maintain that behavior. This runs on both threads; when an > event is enqueued it will run on the cc thread and when we have pop and event > from the main thread it will run if there is remaining work. I do change some of > this for rAF aligned input processing up to a number of events per iteration. > > Thanks. Maybe add a comment indicating in what context this runs on which thread. https://codereview.chromium.org/2162143002/diff/100001/content/renderer/input... File content/renderer/input/main_thread_event_queue.cc (right): https://codereview.chromium.org/2162143002/diff/100001/content/renderer/input... content/renderer/input/main_thread_event_queue.cc:144: std::unique_ptr<EventWithDispatchType>&& event) { We aren't allowed to use && here, are we?
The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tdresser@ please take another look. I had to fix an issue with the trybots failing because of the non-fairness of the post task. Now the SendEventNotificationToMainThread only executes on the compositor thread. https://codereview.chromium.org/2162143002/diff/100001/content/renderer/input... File content/renderer/input/main_thread_event_queue.cc (right): https://codereview.chromium.org/2162143002/diff/100001/content/renderer/input... content/renderer/input/main_thread_event_queue.cc:144: std::unique_ptr<EventWithDispatchType>&& event) { On 2016/08/08 16:56:05, tdresser wrote: > We aren't allowed to use && here, are we? Done.
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Don't use PostTask queueing between compositor and main thread. Instead put the events in the main thread event queue where they can be coalesced. This improves the latency on mouse move events ~5-7X On Linux loading https://jsfiddle.net/va5vpxLz/6/ and getting ~250 mouse samples Stable (RAF) 53.579747191011386 Stable (setTimeout) 27.226215139442306 With Change (RAF) 7.244861660078827 With Change (setTimeout) 5.327854330708663 BUG=624012,599152 ========== to ========== Don't use PostTask queueing between compositor and main thread. Instead put the events in the main thread event queue where they can be coalesced. This improves the latency on mouse move events ~5-7X On Linux loading https://jsfiddle.net/va5vpxLz/6/ and getting ~250 mouse samples Stable (RAF) 53.579747191011386 Stable (setTimeout) 27.226215139442306 With Change (RAF) 7.244861660078827 With Change (setTimeout) 5.327854330708663 BUG=624012,599152 Committed: https://crrev.com/4d87f4e9da90c5fdc94ad48cd6b5c888ef8bd896 Cr-Commit-Position: refs/heads/master@{#410487} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4d87f4e9da90c5fdc94ad48cd6b5c888ef8bd896 Cr-Commit-Position: refs/heads/master@{#410487} |
