Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2715623002:
Add a passthrough touch event queue. (Closed)
DescriptionAdd a passthrough touch event queue.
The passthrough touch event queue does no queuing of sending events. It
does handle the fact that touch events can be ack'd out of order and
it makes it appear to the higher layers in the browsers that the acks
are all in order. ie; You can get an ack for a touchend before a touchstart
from the renderer. If the compositor acks the touchend and the touchstart
is sent to the main thread. This class makes this appear logical to the
browser which relies upon things like the touchstart being ack before the
touchend.
This change allows coalescing to only happen in the main thread event queue.
It also removes the async sending of touchmoves. All touchmoves are
sent right away.
BUG=642368
TBR=clamy@chromium.org
Review-Url: https://codereview.chromium.org/2715623002
Cr-Commit-Position: refs/heads/master@{#453133}
Committed: https://chromium.googlesource.com/chromium/src/+/53f9f4ee5790ce47bf43eee521fdd3536cd1763a
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 23
Patch Set 3 : Fix issues #Patch Set 4 : Rebase on throttling change #
Total comments: 3
Depends on Patchset: Messages
Total messages: 38 (21 generated)
The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run
dtapuska@chromium.org changed reviewers: + tdresser@chromium.org
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/passthrough_touch_event_queue.cc (right): https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.cc:17: #include "ui/gfx/geometry/point_f.h" How thoroughly has this changed from legacy_touch_event_queue.cc? Should I review the whole thing, or are there specific bits to focus on? https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/passthrough_touch_event_queue.h (right): https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.h:16: // A queue for that takes a touch-event and forwards it on to the Wording is wrong here. "A queue for that takes" https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.h:127: std::set<TouchEventWithLatencyInfoAndAckState> outstanding_touches_; Should this be a set? You're referring to the first element of it sometimes... https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc (right): https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:314: protected: Why not private? https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:385: // sent immediately. Sent https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:402: // Ack the touch moves move. This comment is confusing. https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:1701: // Send ACKs Missing . https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:1705: // touch-scroll-start Ack is not reported to client. Capitalize. https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (left): https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:697: TEST_F(TouchEventQueueTest, SpuriousAcksIgnored) { Looks like this is failing sometimes?
https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/passthrough_touch_event_queue.cc (right): https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.cc:17: #include "ui/gfx/geometry/point_f.h" On 2017/02/23 15:10:45, tdresser wrote: > How thoroughly has this changed from legacy_touch_event_queue.cc? > > Should I review the whole thing, or are there specific bits to focus on? Will comment on the interesting bits. https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.cc:87: client_->OnFilteringTouchEvent(event.event); This is interesting code here. The Filtered touch is done right away but the ack can get held onto to complete the sequence correctly. https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.cc:189: drop_remaining_touches_in_sequence_ = true; This is interesting https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.cc:200: void PassthroughTouchEventQueue::AckCompletedEvents() { Interesting https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.cc:279: maybe_has_handler_for_current_sequence_ = true; maybe_ has been added here and inverted. https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.cc:348: // Once we have the ack back for the sequence we know if there This is an interesting block. https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/passthrough_touch_event_queue.h (right): https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.h:16: // A queue for that takes a touch-event and forwards it on to the On 2017/02/23 15:10:45, tdresser wrote: > Wording is wrong here. "A queue for that takes" Done. https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.h:127: std::set<TouchEventWithLatencyInfoAndAckState> outstanding_touches_; On 2017/02/23 15:10:45, tdresser wrote: > Should this be a set? You're referring to the first element of it sometimes... Added comment and adjusted code checking for touch start pending to examine set. https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc (right): https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:314: protected: On 2017/02/23 15:10:45, tdresser wrote: > Why not private? Fixed. I was originally trying to merge the two unit tests into one but that didn't go very well; guess this was left over. https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:385: // sent immediately. On 2017/02/23 15:10:45, tdresser wrote: > Sent I've collapsed it into a single line. https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:402: // Ack the touch moves move. On 2017/02/23 15:10:45, tdresser wrote: > This comment is confusing. Done. https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:1701: // Send ACKs On 2017/02/23 15:10:45, tdresser wrote: > Missing . Done. https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:1705: // touch-scroll-start Ack is not reported to client. On 2017/02/23 15:10:45, tdresser wrote: > Capitalize. Done. https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (left): https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:697: TEST_F(TouchEventQueueTest, SpuriousAcksIgnored) { On 2017/02/23 15:10:45, tdresser wrote: > Looks like this is failing sometimes? Acknowledged.
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: This issue passed the CQ dry run.
Description was changed from ========== Add a passthrough touch event queue. The passthrough touch event queue does no queuing of sending events. It does handle the fact that touch events can be ack'd out of order and it makes it appear to the higher layers in the browsers that the acks are all in order. ie; You can get an ack for a touchend before a touchstart from the renderer. If the compositor acks the touchend and the touchstart is sent to the main thread. This class makes this appear logical to the browser which relies upon things like the touchstart being ack before the touchend. This change allows coalescing to only happen in the main thread event queue. It also removes the async sending of touchmoves. All touchmoves are sent right away. BUG=642368 ========== to ========== Add a passthrough touch event queue. The passthrough touch event queue does no queuing of sending events. It does handle the fact that touch events can be ack'd out of order and it makes it appear to the higher layers in the browsers that the acks are all in order. ie; You can get an ack for a touchend before a touchstart from the renderer. If the compositor acks the touchend and the touchstart is sent to the main thread. This class makes this appear logical to the browser which relies upon things like the touchstart being ack before the touchend. This change allows coalescing to only happen in the main thread event queue. It also removes the async sending of touchmoves. All touchmoves are sent right away. BUG=642368 ==========
dtapuska@chromium.org changed reviewers: + rbyers@chromium.org
On 2017/02/23 16:52:12, dtapuska wrote: > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/input/passthrough_touch_event_queue.cc > (right): > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/passthrough_touch_event_queue.cc:17: > #include "ui/gfx/geometry/point_f.h" > On 2017/02/23 15:10:45, tdresser wrote: > > How thoroughly has this changed from legacy_touch_event_queue.cc? > > > > Should I review the whole thing, or are there specific bits to focus on? > > Will comment on the interesting bits. > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/passthrough_touch_event_queue.cc:87: > client_->OnFilteringTouchEvent(event.event); > This is interesting code here. The Filtered touch is done right away but the ack > can get held onto to complete the sequence correctly. > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/passthrough_touch_event_queue.cc:189: > drop_remaining_touches_in_sequence_ = true; > This is interesting > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/passthrough_touch_event_queue.cc:200: void > PassthroughTouchEventQueue::AckCompletedEvents() { > Interesting > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/passthrough_touch_event_queue.cc:279: > maybe_has_handler_for_current_sequence_ = true; > maybe_ has been added here and inverted. > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/passthrough_touch_event_queue.cc:348: // > Once we have the ack back for the sequence we know if there > This is an interesting block. > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/input/passthrough_touch_event_queue.h > (right): > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/passthrough_touch_event_queue.h:16: // A > queue for that takes a touch-event and forwards it on to the > On 2017/02/23 15:10:45, tdresser wrote: > > Wording is wrong here. "A queue for that takes" > > Done. > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/passthrough_touch_event_queue.h:127: > std::set<TouchEventWithLatencyInfoAndAckState> outstanding_touches_; > On 2017/02/23 15:10:45, tdresser wrote: > > Should this be a set? You're referring to the first element of it sometimes... > > Added comment and adjusted code checking for touch start pending to examine set. > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > File > content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc > (right): > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:314: > protected: > On 2017/02/23 15:10:45, tdresser wrote: > > Why not private? > > Fixed. I was originally trying to merge the two unit tests into one but that > didn't go very well; guess this was left over. > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:385: > // sent immediately. > On 2017/02/23 15:10:45, tdresser wrote: > > Sent > > I've collapsed it into a single line. > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:402: > // Ack the touch moves move. > On 2017/02/23 15:10:45, tdresser wrote: > > This comment is confusing. > > Done. > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:1701: > // Send ACKs > On 2017/02/23 15:10:45, tdresser wrote: > > Missing . > > Done. > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:1705: > // touch-scroll-start Ack is not reported to client. > On 2017/02/23 15:10:45, tdresser wrote: > > Capitalize. > > Done. > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/input/touch_event_queue_unittest.cc (left): > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/touch_event_queue_unittest.cc:697: > TEST_F(TouchEventQueueTest, SpuriousAcksIgnored) { > On 2017/02/23 15:10:45, tdresser wrote: > > Looks like this is failing sometimes? > > Acknowledged. rbyers@ this gets rid of the async touch move throttling. So it does mean that we are coupling the release of rAF aligned input with decoupling the throttling of events during scroll. I think we have a good history of the trial data of the rAF aligned input (it has been on in canary/dev for a few months) so the coupling isn't a grand cause for concern. But would like your thoughts.
On 2017/02/23 18:03:09, dtapuska wrote: > On 2017/02/23 16:52:12, dtapuska wrote: > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > File content/browser/renderer_host/input/passthrough_touch_event_queue.cc > > (right): > > > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > content/browser/renderer_host/input/passthrough_touch_event_queue.cc:17: > > #include "ui/gfx/geometry/point_f.h" > > On 2017/02/23 15:10:45, tdresser wrote: > > > How thoroughly has this changed from legacy_touch_event_queue.cc? > > > > > > Should I review the whole thing, or are there specific bits to focus on? > > > > Will comment on the interesting bits. > > > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > content/browser/renderer_host/input/passthrough_touch_event_queue.cc:87: > > client_->OnFilteringTouchEvent(event.event); > > This is interesting code here. The Filtered touch is done right away but the > ack > > can get held onto to complete the sequence correctly. > > > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > content/browser/renderer_host/input/passthrough_touch_event_queue.cc:189: > > drop_remaining_touches_in_sequence_ = true; > > This is interesting > > > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > content/browser/renderer_host/input/passthrough_touch_event_queue.cc:200: void > > PassthroughTouchEventQueue::AckCompletedEvents() { > > Interesting > > > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > content/browser/renderer_host/input/passthrough_touch_event_queue.cc:279: > > maybe_has_handler_for_current_sequence_ = true; > > maybe_ has been added here and inverted. > > > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > content/browser/renderer_host/input/passthrough_touch_event_queue.cc:348: // > > Once we have the ack back for the sequence we know if there > > This is an interesting block. > > > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > File content/browser/renderer_host/input/passthrough_touch_event_queue.h > > (right): > > > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > content/browser/renderer_host/input/passthrough_touch_event_queue.h:16: // A > > queue for that takes a touch-event and forwards it on to the > > On 2017/02/23 15:10:45, tdresser wrote: > > > Wording is wrong here. "A queue for that takes" > > > > Done. > > > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > content/browser/renderer_host/input/passthrough_touch_event_queue.h:127: > > std::set<TouchEventWithLatencyInfoAndAckState> outstanding_touches_; > > On 2017/02/23 15:10:45, tdresser wrote: > > > Should this be a set? You're referring to the first element of it > sometimes... > > > > Added comment and adjusted code checking for touch start pending to examine > set. > > > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > File > > content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc > > (right): > > > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > > content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:314: > > protected: > > On 2017/02/23 15:10:45, tdresser wrote: > > > Why not private? > > > > Fixed. I was originally trying to merge the two unit tests into one but that > > didn't go very well; guess this was left over. > > > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > > content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:385: > > // sent immediately. > > On 2017/02/23 15:10:45, tdresser wrote: > > > Sent > > > > I've collapsed it into a single line. > > > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > > content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:402: > > // Ack the touch moves move. > > On 2017/02/23 15:10:45, tdresser wrote: > > > This comment is confusing. > > > > Done. > > > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > > content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:1701: > > // Send ACKs > > On 2017/02/23 15:10:45, tdresser wrote: > > > Missing . > > > > Done. > > > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > > content/browser/renderer_host/input/passthrough_touch_event_queue_unittest.cc:1705: > > // touch-scroll-start Ack is not reported to client. > > On 2017/02/23 15:10:45, tdresser wrote: > > > Capitalize. > > > > Done. > > > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > File content/browser/renderer_host/input/touch_event_queue_unittest.cc (left): > > > > > https://codereview.chromium.org/2715623002/diff/20001/content/browser/rendere... > > content/browser/renderer_host/input/touch_event_queue_unittest.cc:697: > > TEST_F(TouchEventQueueTest, SpuriousAcksIgnored) { > > On 2017/02/23 15:10:45, tdresser wrote: > > > Looks like this is failing sometimes? > > > > Acknowledged. > > rbyers@ this gets rid of the async touch move throttling. So it does mean that > we are coupling the release of rAF aligned input with decoupling the throttling > of events during scroll. I think we have a good history of the trial data of the > rAF aligned input (it has been on in canary/dev for a few months) so the > coupling isn't a grand cause for concern. But would like your thoughts. The throttle was just a mitigation against a hypothetical performance regression. So if you have finch trial data showing no significant perf regression (eg. in TouchToScrollUpdateSwap metrics) then I think it's fine to remove (though I'd ask aelias@ his opinion as well since he was involved in the throttle design). Removing it gets us closer to Safari, which is a good thing. It also may make some issues associated with your intervention less obvious/ugly (people complain of jerky dragging) - which I _think_ is net positive. The throttle was also done before we had the blink scheduler. Now that we have the scheduler, if an increased touchmove-during-scroll rate does cause increased scroll jank, that's probably a scheduler bug that is valuable to fix regardless.
dtapuska@chromium.org changed reviewers: + aelias@chromium.org
aelias@ do you have any opinion related to my comments?
The problem throttling-during-scroll fixes is not really hypothetical, it's more that it was observed in manual traces but we don't necessarily have a metric designed to clearly catch it. The issue is stuff like ads using touch events during scroll to do heavy DOM analysis to calculate visibility, as a poor man's IntersectionObserver. The main thread could wind up churning an entire CPU core on this useless work during scrolling. Even if it doesn't result in scroll jank due to compositor-thread isolation, that's still really wasteful. You can think of it as a preexisting intervention in that sense. So I'd prefer it be decoupled and that we separately make the decision of how to weigh this kind of waste vs the use cases like parallax background. How hard would it be to do it in the new model? Wouldn't it just require sampling the MainThreadEventQueue at a slower frequency than every vsync, if a scrolling boolean is present?
On 2017/02/23 22:17:29, aelias wrote: > The problem throttling-during-scroll fixes is not really hypothetical, it's more > that it was observed in manual traces but we don't necessarily have a metric > designed to clearly catch it. The issue is stuff like ads using touch events > during scroll to do heavy DOM analysis to calculate visibility, as a poor man's > IntersectionObserver. The main thread could wind up churning an entire CPU core > on this useless work during scrolling. Even if it doesn't result in scroll jank > due to compositor-thread isolation, that's still really wasteful. You can think > of it as a preexisting intervention in that sense. > > So I'd prefer it be decoupled and that we separately make the decision of how to > weigh this kind of waste vs the use cases like parallax background. How hard > would it be to do it in the new model? Wouldn't it just require sampling the > MainThreadEventQueue at a slower frequency than every vsync, if a scrolling > boolean is present? Yes that was what I was telling tdresser is that we can just skip some raf callbacks if we wanted this behaviour. I think the question is do we really want this behaviour. I guess I can implement it as a new model and then decided to get rid of it as a separate thing?
On 2017/02/23 at 22:22:38, dtapuska wrote: > Yes that was what I was telling tdresser is that we can just skip some raf callbacks if we wanted this behaviour. I think the question is do we really want this behaviour. I guess I can implement it as a new model and then decided to get rid of it as a separate thing? Yeah, I'd prefer we keep it and make a more informed isolated decision about reverting it based on arguments like A) the ecosystem has improved and B) we don't have a sufficient alternative story for the use cases. High-frequency touch and scroll events are prone to abuse and that kept coming up in traces back when they weren't throttled. The fact that the page is scrolling is typically a pretty strong signal that it's some niche feature that doesn't really need the frequency, as opposed to something like a paint or map app that does.
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...
On 2017/02/23 22:32:36, aelias wrote: > On 2017/02/23 at 22:22:38, dtapuska wrote: > > Yes that was what I was telling tdresser is that we can just skip some raf > callbacks if we wanted this behaviour. I think the question is do we really want > this behaviour. I guess I can implement it as a new model and then decided to > get rid of it as a separate thing? > > Yeah, I'd prefer we keep it and make a more informed isolated decision about > reverting it based on arguments like A) the ecosystem has improved and B) we > don't have a sufficient alternative story for the use cases. High-frequency > touch and scroll events are prone to abuse and that kept coming up in traces > back when they weren't throttled. The fact that the page is scrolling is > typically a pretty strong signal that it's some niche feature that doesn't > really need the frequency, as opposed to something like a paint or map app that > does. tdresser@ can you review this now that I have rebased it off the other CL that introduces throttling to the main thread event queue
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2715623002/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/passthrough_touch_event_queue.cc (right): https://codereview.chromium.org/2715623002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.cc:9: #include "base/auto_reset.h" Are we using all these includes? https://codereview.chromium.org/2715623002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.cc:28: bool HasPointChanged(const WebTouchPoint& point_1, Comment doesn't really add anything. The name implies we're looking at change over time, but the parameter names and behavior don't. Should this should be the == operator on points? Or ArePointsEqual, instead of HasPointChanged? https://codereview.chromium.org/2715623002/diff/60001/content/browser/rendere... content/browser/renderer_host/input/passthrough_touch_event_queue.cc:52: return event.uniqueTouchEventId < other.event.uniqueTouchEventId; Every 2 years or so of use, we'll wrap around the unique id, and this will horribly break, right? We could add some failsafe, where we try to check if it wrapped around. Not sure if it's worth it though. Or could we use the timestamp to avoid this?
On 2017/02/23 22:32:36, aelias wrote: > On 2017/02/23 at 22:22:38, dtapuska wrote: > > Yes that was what I was telling tdresser is that we can just skip some raf > callbacks if we wanted this behaviour. I think the question is do we really want > this behaviour. I guess I can implement it as a new model and then decided to > get rid of it as a separate thing? > > Yeah, I'd prefer we keep it and make a more informed isolated decision about > reverting it based on arguments like A) the ecosystem has improved and B) we > don't have a sufficient alternative story for the use cases. High-frequency > touch and scroll events are prone to abuse and that kept coming up in traces > back when they weren't throttled. The fact that the page is scrolling is > typically a pretty strong signal that it's some niche feature that doesn't > really need the frequency, as opposed to something like a paint or map app that > does. Sorry I didn't realize there were known cases where this was a real problem. Note that Safari does not appear to explicitly throttle touchmoves during a scroll. Since we believe our 95th percentile TouchToScrollUpdateSwap data represents high CPU load cases (likely thermally throttled), I'd expect any additional CPU load to move those metrics. We believe we've seen this in the data for the scroll intervention in M56 - now that we can scroll more often during periods of busy CPU, overall scroll latency has gone up a little. It should also move the estimated queuing time metrics, right? More tasks / CPU usage == longer estimated queuing time. Dave, are you saying that the plan now is not to throttle the touchmove events, but to throttle the whole rendering pipeline on the main thread during a scroll (rAF callbacks etc.)? That obviously has some bigger implications.
On 2017/02/24 20:52:13, Rick Byers wrote: > On 2017/02/23 22:32:36, aelias wrote: > > On 2017/02/23 at 22:22:38, dtapuska wrote: > > > Yes that was what I was telling tdresser is that we can just skip some raf > > callbacks if we wanted this behaviour. I think the question is do we really > want > > this behaviour. I guess I can implement it as a new model and then decided to > > get rid of it as a separate thing? > > > > Yeah, I'd prefer we keep it and make a more informed isolated decision about > > reverting it based on arguments like A) the ecosystem has improved and B) we > > don't have a sufficient alternative story for the use cases. High-frequency > > touch and scroll events are prone to abuse and that kept coming up in traces > > back when they weren't throttled. The fact that the page is scrolling is > > typically a pretty strong signal that it's some niche feature that doesn't > > really need the frequency, as opposed to something like a paint or map app > that > > does. > > Sorry I didn't realize there were known cases where this was a real problem. > Note that Safari does not appear to explicitly throttle touchmoves during a > scroll. Since we believe our 95th percentile TouchToScrollUpdateSwap data > represents high CPU load cases (likely thermally throttled), I'd expect any > additional CPU load to move those metrics. We believe we've seen this in the > data for the scroll intervention in M56 - now that we can scroll more often > during periods of busy CPU, overall scroll latency has gone up a little. It > should also move the estimated queuing time metrics, right? More tasks / CPU > usage == longer estimated queuing time. > > Dave, are you saying that the plan now is not to throttle the touchmove events, > but to throttle the whole rendering pipeline on the main thread during a scroll > (rAF callbacks etc.)? That obviously has some bigger implications. No not throttle the whole rendering pipeline. We just pretend there is no input if we have only a single non-cancelable touch move in the queue that has exceeded the slop region.
On 2017/02/24 20:58:36, dtapuska wrote: > On 2017/02/24 20:52:13, Rick Byers wrote: > > On 2017/02/23 22:32:36, aelias wrote: > > > On 2017/02/23 at 22:22:38, dtapuska wrote: > > > > Yes that was what I was telling tdresser is that we can just skip some raf > > > callbacks if we wanted this behaviour. I think the question is do we really > > want > > > this behaviour. I guess I can implement it as a new model and then decided > to > > > get rid of it as a separate thing? > > > > > > Yeah, I'd prefer we keep it and make a more informed isolated decision about > > > reverting it based on arguments like A) the ecosystem has improved and B) we > > > don't have a sufficient alternative story for the use cases. High-frequency > > > touch and scroll events are prone to abuse and that kept coming up in traces > > > back when they weren't throttled. The fact that the page is scrolling is > > > typically a pretty strong signal that it's some niche feature that doesn't > > > really need the frequency, as opposed to something like a paint or map app > > that > > > does. > > > > Sorry I didn't realize there were known cases where this was a real problem. > > Note that Safari does not appear to explicitly throttle touchmoves during a > > scroll. Since we believe our 95th percentile TouchToScrollUpdateSwap data > > represents high CPU load cases (likely thermally throttled), I'd expect any > > additional CPU load to move those metrics. We believe we've seen this in the > > data for the scroll intervention in M56 - now that we can scroll more often > > during periods of busy CPU, overall scroll latency has gone up a little. It > > should also move the estimated queuing time metrics, right? More tasks / CPU > > usage == longer estimated queuing time. > > > > Dave, are you saying that the plan now is not to throttle the touchmove > events, > > but to throttle the whole rendering pipeline on the main thread during a > scroll > > (rAF callbacks etc.)? That obviously has some bigger implications. > > No not throttle the whole rendering pipeline. We just pretend there is no input > if we have only a single non-cancelable touch move in the queue that has > exceeded the slop region. Ah, thanks - that makes more sense and sounds fine to me for now. I was confused by your "skip rAF callbacks" comment.
dtapuska@chromium.org changed reviewers: + clamy@chromium.org - aelias@chromium.org, rbyers@chromium.org
clamy@chromium.org: Please review changes in BUIDL.gn
Description was changed from ========== Add a passthrough touch event queue. The passthrough touch event queue does no queuing of sending events. It does handle the fact that touch events can be ack'd out of order and it makes it appear to the higher layers in the browsers that the acks are all in order. ie; You can get an ack for a touchend before a touchstart from the renderer. If the compositor acks the touchend and the touchstart is sent to the main thread. This class makes this appear logical to the browser which relies upon things like the touchstart being ack before the touchend. This change allows coalescing to only happen in the main thread event queue. It also removes the async sending of touchmoves. All touchmoves are sent right away. BUG=642368 ========== to ========== Add a passthrough touch event queue. The passthrough touch event queue does no queuing of sending events. It does handle the fact that touch events can be ack'd out of order and it makes it appear to the higher layers in the browsers that the acks are all in order. ie; You can get an ack for a touchend before a touchstart from the renderer. If the compositor acks the touchend and the touchstart is sent to the main thread. This class makes this appear logical to the browser which relies upon things like the touchstart being ack before the touchend. This change allows coalescing to only happen in the main thread event queue. It also removes the async sending of touchmoves. All touchmoves are sent right away. BUG=642368 TBR=clamy@chromium.org ==========
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...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488154828494480,
"parent_rev": "f26fc7214b00902aaa67a6ebf96e2e91a1ddd028", "commit_rev":
"53f9f4ee5790ce47bf43eee521fdd3536cd1763a"}
Message was sent while issue was closed.
Description was changed from ========== Add a passthrough touch event queue. The passthrough touch event queue does no queuing of sending events. It does handle the fact that touch events can be ack'd out of order and it makes it appear to the higher layers in the browsers that the acks are all in order. ie; You can get an ack for a touchend before a touchstart from the renderer. If the compositor acks the touchend and the touchstart is sent to the main thread. This class makes this appear logical to the browser which relies upon things like the touchstart being ack before the touchend. This change allows coalescing to only happen in the main thread event queue. It also removes the async sending of touchmoves. All touchmoves are sent right away. BUG=642368 TBR=clamy@chromium.org ========== to ========== Add a passthrough touch event queue. The passthrough touch event queue does no queuing of sending events. It does handle the fact that touch events can be ack'd out of order and it makes it appear to the higher layers in the browsers that the acks are all in order. ie; You can get an ack for a touchend before a touchstart from the renderer. If the compositor acks the touchend and the touchstart is sent to the main thread. This class makes this appear logical to the browser which relies upon things like the touchstart being ack before the touchend. This change allows coalescing to only happen in the main thread event queue. It also removes the async sending of touchmoves. All touchmoves are sent right away. BUG=642368 TBR=clamy@chromium.org Review-Url: https://codereview.chromium.org/2715623002 Cr-Commit-Position: refs/heads/master@{#453133} Committed: https://chromium.googlesource.com/chromium/src/+/53f9f4ee5790ce47bf43eee521fd... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/53f9f4ee5790ce47bf43eee521fd... |
