Introduce DispatchScrollEventsUntilDeadline [chromium/src : main]
Jonathan Ross (Gerrit)
Jonathan Ross added 1 comment![]( "Open in Gerrit")
Hey,
I wanted your early thoughts on this. I'm giving thought on how to have `InputHandlerProxy::DeliverInputForBeginFrame` not throttle on devices that are constantly over the deadline.
I decided to upload early to also run vs the benchmarks.
Related details
- Victor Miura
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/178ee2dc210000
Related details
- Jonathan Ross
- Victor Miura
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross added 4 comments![]( "Open in Gerrit")
DispatchScrollEventsImmediately. Which shows signifcant improvements to"signifcant" is a possible misspelling of "significant".
Please fix.
However it encounters issues on platforms/devices with a signle dicrete"signle" is a possible misspelling of "single" or "signal".
Please fix.
However it encounters issues on platforms/devices with a signle dicrete"dicrete" is a possible misspelling of "discrete".
Please fix.
// We likely need to cap the number of consequtive times duing which this"consequtive" is a possible misspelling of "consecutive".
Please fix.
Related details
- Victor Miura
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross added 1 comment![]( "Open in Gerrit")
// We likely need to cap the number of consequtive times duing which this"consequtive" is a possible misspelling of "consecutive".
Please fix.
Related details
- Victor Miura
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross added 1 comment![]( "Open in Gerrit")
Hey, I plan to add a unit test to this, but the core feature is ready for review
Related details
- Victor Miura
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross added 1 comment![]( "Open in Gerrit")
Added unittest coverage. Could you PTAL? Thanks!
Related details
- Victor Miura
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Miura (Gerrit)
Victor Miura added 3 comments![]( "Open in Gerrit")
Looks good. A couple of questions in cases that are not currently covered.
void InputHandlerProxy::ContinueScrollBeginAfterMainThreadHitTest(Do we need to avoid dispatching late events in cases like this too?
DispatchQueuedInputEvents(false /* frame_aligned */);What do we expect in this case for high latency mode?
Related details
- Jonathan Ross
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross added 3 comments![]( "Open in Gerrit")
Since the new delay is for main thread targeting gonna run a perf test vs tough_scrolling_cases/text_on_non_opaque_background.html to see effect on main thread scrolling
void InputHandlerProxy::ContinueScrollBeginAfterMainThreadHitTest(Do we need to avoid dispatching late events in cases like this too?
Good catch. I'm adding the delay at the bottom, to where we attempt to run any `scroll updates` that are queued.
I think since the `callback` is for the Browser process, that we should run that immediately. The other side effects here are setting that we are in a scrolling state. Which we'd want for queueing/scheduling decisions.
DispatchQueuedInputEvents(false /* frame_aligned */);What do we expect in this case for high latency mode?
I think this method is actually dead code. `ResamplingScrollEvents` has been enabled by default since 2023. So we should always have a scroll predictor.
I can do a follow up to clean up this class some. Should make plan to refactor it to `components/input` easier too.
Though in general I would +1 to not handling input after commit completes
Related details
- Victor Miura
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Victor Miura
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross voted Code-Review+0![]( "Open in Gerrit")
| Code-Review | +0 |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1663f03c210000
Related details
- Jonathan Ross
- Victor Miura
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross added 1 comment![]( "Open in Gerrit")
void InputHandlerProxy::ContinueScrollBeginAfterMainThreadHitTest(Jonathan RossDo we need to avoid dispatching late events in cases like this too?
Good catch. I'm adding the delay at the bottom, to where we attempt to run any `scroll updates` that are queued.
I think since the `callback` is for the Browser process, that we should run that immediately. The other side effects here are setting that we are in a scrolling state. Which we'd want for queueing/scheduling decisions.
Pinpoint didn't like the extra arg I was trying to set, so a local repro on macOS of `non_opaque_background_main_thread_scrolling_00050_pixels_per_second` does validate that this change isn't breaking the behaviour of main thread targetting.
The variant on the pinpoint apparently now needs `--disable-features=RasterInducingScroll` added to the story. As that feature gets scrolling on compositor thread for this case now. I'll update that separately.
Related details
- Victor Miura
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross added 1 comment![]( "Open in Gerrit")
Jonathan Ross (Gerrit)
Jonathan Ross added 1 comment![]( "Open in Gerrit")
- Ian Vollick
- Victor Miura
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross added 5 comments![]( "Open in Gerrit")
// same as `kScrollEventDispatchModeDispatchScrollEventsImmediately` until the
// deadline is encountered. At which point input will no longer be dispatched,input handler proxy enforcing the deadline itself
// deadline we will resume frame production and enqueuing input.
//cc::schedule deadline, overridden if waiting for main thread content
// During this time scroll events will be dispatched immediately. At theThe first scroll event. Subsequent will be enqueued
void InputHandlerProxy::ContinueScrollBeginAfterMainThreadHitTest(Jonathan RossDo we need to avoid dispatching late events in cases like this too?
Jonathan RossGood catch. I'm adding the delay at the bottom, to where we attempt to run any `scroll updates` that are queued.
I think since the `callback` is for the Browser process, that we should run that immediately. The other side effects here are setting that we are in a scrolling state. Which we'd want for queueing/scheduling decisions.
Pinpoint didn't like the extra arg I was trying to set, so a local repro on macOS of `non_opaque_background_main_thread_scrolling_00050_pixels_per_second` does validate that this change isn't breaking the behaviour of main thread targetting.
The variant on the pinpoint apparently now needs `--disable-features=RasterInducingScroll` added to the story. As that feature gets scrolling on compositor thread for this case now. I'll update that separately.
Done
DispatchQueuedInputEvents(false /* frame_aligned */);Jonathan RossWhat do we expect in this case for high latency mode?
I think this method is actually dead code. `ResamplingScrollEvents` has been enabled by default since 2023. So we should always have a scroll predictor.
I can do a follow up to clean up this class some. Should make plan to refactor it to `components/input` easier too.
Though in general I would +1 to not handling input after commit completes
Done
Related details
- Ian Vollick
- Victor Miura
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick added 9 comments![]( "Open in Gerrit")
// from scheduling bugs. Allowing us to no longer dispatch events, even if frameNit: "cc scheduling bugs".
// input. During this time the first scroll event will be dispatchedIt looks like we could call DispatchQueuedInputEvents, which looks like it'll dispatch a number of single events. Is that right?
bool DoNotEnqueueLateScrollEvents(Does this mean "ImmediatelyDispatchFirstScrollEventBeforDeadline"? (% question above about a single event -- similar for my other comments that reference the "first" event).
The DoNotEnqueue threw me given that we do enqueue after the first event (prior to the deadline), and we always enqueue after the deadline.
bool ShouldNotDispatchLateInputEvent(nit: The early return if the feature is disabled combined with the function name make this a bit of a confusing double negative.
Wdyt of ForceImmediateDispatchOfFirstLateInputEvent?
That way
* if the feature is off, we return true (IHP forces dispatch counting on someone else to handle gating dispatch, if needed)
* if we're >= one VSync out, we force (no sense waiting)
* otherwise, we return whether or not the frame delta < deadline.
kDispatchScrollEventsUntilDeadline) {If we keep the function named as is, could you please add a comment here explaining this this will cause us to dispatch (and why that's ok)?
// DeliverInputForBeginFrame we want to dispatch those immediately. We wills/those/the first event/ ?
synchronous_input_handler_) {Just to confirm my understanding, the next time we hit this fn after hitting the if block above (and have set `enqueue_scroll_events_ = true`), we'll not enter the if block above and will instead end up here.
We could still end up dispatching here, though, if it happens to be the scroll end from wheel (wouldn't be the first wheel scroll because this is the 2nd time hitting this fn), or if this is WebView.
Is that right? If so, enqueue_scroll_events_ seems a bit confusing and might warrant a comment here.
enqueue_scroll_events_ = true;I was a bit surprised to see that we don't return in this case. Is the idea that we'd generate an extra synthetic event and then adjust and dispatch the others in the while loop below?
DispatchQueuedInputEvents(false /* frame_aligned */);Do we need to gate this, too?
Related details
- Jonathan Ross
- Victor Miura
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jonathan Ross added 8 comments![]( "Open in Gerrit")
// input. During this time the first scroll event will be dispatchedIt looks like we could call DispatchQueuedInputEvents, which looks like it'll dispatch a number of single events. Is that right?
It's more "Dispatch instead of queue" and "only if we are close to vysnc" mode.
The system was setup expecting all input to arrive before a VSync. So it was queueing them, then dispatching the queue during the VSync.
The failure case is when we get to the VSync and the queue is empty. So `kScrollEventDispatchModeNameDispatchScrollEventsImmediately` waits and performs "Dispatch instead of queue".
However if that's too late, we end up backing up pipeline and dropping frames. So `kScrollEventDispatchModeDispatchScrollEventsUntilDeadline` checks "if we are close to vysnc". Otherwise it queues
Does this mean "ImmediatelyDispatchFirstScrollEventBeforDeadline"? (% question above about a single event -- similar for my other comments that reference the "first" event).
The DoNotEnqueue threw me given that we do enqueue after the first event (prior to the deadline), and we always enqueue after the deadline.
Yeah the name was from when we were either "dispatch immediately" or "enqueue". However on devices with two inputs per vsync this was problematic.
So this is effectively "ImmediatelyDispatchFirstScrollEventBeforeDeadline". Renamed.
bool ShouldNotDispatchLateInputEvent(nit: The early return if the feature is disabled combined with the function name make this a bit of a confusing double negative.
Wdyt of ForceImmediateDispatchOfFirstLateInputEvent?
That way
* if the feature is off, we return true (IHP forces dispatch counting on someone else to handle gating dispatch, if needed)
* if we're >= one VSync out, we force (no sense waiting)
* otherwise, we return whether or not the frame delta < deadline.
I knew it was a bit of a double negative. However I felt the inverse would have just required negation in the if statements anyways.
eg
```
if (!ForceImmediateDispatchOfFirstLateInputEvent(
scroll_event_dispatch_mode_, scroll_deadline_ratio_,
current_begin_frame_args_, tick_clock_)) {
input_handler_->SetNeedsAnimateInput();
return;
}
```
If we keep the function named as is, could you please add a comment here explaining this this will cause us to dispatch (and why that's ok)?
Done
// DeliverInputForBeginFrame we want to dispatch those immediately. We willJonathan Rosss/those/the first event/ ?
Done
synchronous_input_handler_) {Just to confirm my understanding, the next time we hit this fn after hitting the if block above (and have set `enqueue_scroll_events_ = true`), we'll not enter the if block above and will instead end up here.
We could still end up dispatching here, though, if it happens to be the scroll end from wheel (wouldn't be the first wheel scroll because this is the 2nd time hitting this fn), or if this is WebView.
Is that right? If so, enqueue_scroll_events_ seems a bit confusing and might warrant a comment here.
That is correct. Adding a note.
Interestingly at `line 401` above bokan@ mentions that this might not be needed anymore for scroll wheel.
Separately, since WebView just always forwards, a lot of this class is kinda redundant. It might be worth keeping the core targeting/dispatch code. Then have the receiving/queueing be in different sub-classes.
enqueue_scroll_events_ = true;I was a bit surprised to see that we don't return in this case. Is the idea that we'd generate an extra synthetic event and then adjust and dispatch the others in the while loop below?
So this does perform the `DispatchSingleInputEvent`. However we may still have `queue_flushed_callback_` to fire.
With `!scroll_predictor_` being not a mode anymore, I can remove that in the feature cleanup. Which means this block could become an explicit `else` with the `while` below it. Might be more clear
DispatchQueuedInputEvents(false /* frame_aligned */);Do we need to gate this, too?
Actually no. `scroll_predictor_` is only `null` when `ResamplingScrollEvents` is disabled. However that experiment was enabled by default over a year ago.
I plan to go through and clean up that feature. So this path will be removed
Related details
- Ian Vollick
- Victor Miura
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Vollick (Gerrit)
Ian Vollick added 5 comments![]( "Open in Gerrit")
// input. During this time the first scroll event will be dispatchedJonathan RossIt looks like we could call DispatchQueuedInputEvents, which looks like it'll dispatch a number of single events. Is that right?
It's more "Dispatch instead of queue" and "only if we are close to vysnc" mode.
The system was setup expecting all input to arrive before a VSync. So it was queueing them, then dispatching the queue during the VSync.
The failure case is when we get to the VSync and the queue is empty. So `kScrollEventDispatchModeNameDispatchScrollEventsImmediately` waits and performs "Dispatch instead of queue".
However if that's too late, we end up backing up pipeline and dropping frames. So `kScrollEventDispatchModeDispatchScrollEventsUntilDeadline` checks "if we are close to vysnc". Otherwise it queues
I think I follow, though when we dispatch instead of queue, could we not dispatch multiple events? I'm asking because we've now switched to using "first scroll event"-type terminology and I wanted to double check that it is actually the first event (and not multiple events) that we might dispatch.
bool ShouldNotDispatchLateInputEvent(Jonathan Rossnit: The early return if the feature is disabled combined with the function name make this a bit of a confusing double negative.
Wdyt of ForceImmediateDispatchOfFirstLateInputEvent?
That way
* if the feature is off, we return true (IHP forces dispatch counting on someone else to handle gating dispatch, if needed)
* if we're >= one VSync out, we force (no sense waiting)
* otherwise, we return whether or not the frame delta < deadline.
I knew it was a bit of a double negative. However I felt the inverse would have just required negation in the if statements anyways.
eg
```
if (!ForceImmediateDispatchOfFirstLateInputEvent(
scroll_event_dispatch_mode_, scroll_deadline_ratio_,
current_begin_frame_args_, tick_clock_)) {
input_handler_->SetNeedsAnimateInput();
return;
}
```
Acknowledged
synchronous_input_handler_) {Jonathan RossJust to confirm my understanding, the next time we hit this fn after hitting the if block above (and have set `enqueue_scroll_events_ = true`), we'll not enter the if block above and will instead end up here.
We could still end up dispatching here, though, if it happens to be the scroll end from wheel (wouldn't be the first wheel scroll because this is the 2nd time hitting this fn), or if this is WebView.
Is that right? If so, enqueue_scroll_events_ seems a bit confusing and might warrant a comment here.
That is correct. Adding a note.
Interestingly at `line 401` above bokan@ mentions that this might not be needed anymore for scroll wheel.
Separately, since WebView just always forwards, a lot of this class is kinda redundant. It might be worth keeping the core targeting/dispatch code. Then have the receiving/queueing be in different sub-classes.
Thanks for adding the note.
enqueue_scroll_events_ = true;Jonathan RossI was a bit surprised to see that we don't return in this case. Is the idea that we'd generate an extra synthetic event and then adjust and dispatch the others in the while loop below?
So this does perform the `DispatchSingleInputEvent`. However we may still have `queue_flushed_callback_` to fire.
With `!scroll_predictor_` being not a mode anymore, I can remove that in the feature cleanup. Which means this block could become an explicit `else` with the `while` below it. Might be more clear
That makes sense, though I'm more wondering about the while loop where we're dealing with the events from compositor_event_queue_. If we've generated and dispatched the synthetic scroll prediction, I'm wondering the purpose of dispatching the events in compositor_event_queue_ (do we need to flush the queue or could we just issue the callback and empty the queue?)
DispatchQueuedInputEvents(false /* frame_aligned */);Jonathan RossDo we need to gate this, too?
Actually no. `scroll_predictor_` is only `null` when `ResamplingScrollEvents` is disabled. However that experiment was enabled by default over a year ago.
I plan to go through and clean up that feature. So this path will be removed
- Jonathan Ross
- Victor Miura
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |