[event-timing] Improve grouping by frame index and fix iteration. [chromium/src : main]
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
event_timing_entries_.clear();Just because we might be in the middle of a stack with multiple event timing processing, it might be best not to touch this vector.
We don't call ReportEventTiming() from processing in this patch, but there is an upcoming change that will add that...
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel added 6 comments![]( "Open in Gerrit")
uint64_t frame_index = 0;Is it worthwhile saying somewhere what is the smallest "real" frame index that we expect?
// Presentation promise index in which the entry in |event_timing_| was
// added.Is this comment out of date?
uint64_t current_frame_index_ = 1;From this, I gather that the smallest 'real' frame index may be 1, is that right?
entry->GetEventTimingReportingInfo()->frame_index = current_frame_index_;Would it be possible to set frame_index on reporting_info prior to PerformanceEventTiming::Create?
If so, it seems that reporting_info is a local variable, so it could be more efficient to std::move it when ::Create is getting called actually - I think we're making a copy then, right now?
current_frame_index_++;I'm a bit surprised this happens in this method. Doesn't this mean that only when there are event timings without next paint (this method is invoked conditionally) will the thingy get incremented?
It would be more expected to have this increment in the caller of this method?
I see there's already another one in OnPaintFinished.
UNSAFE_BUFFERS(base::span(all_entries.begin(), end_of_frame_it));I wonder if this could be done with subspan instead, without requiring the allcaps UNSAFE.
If this std::ranges thingy in l. 940 could be reformulated to directly produce the length of the subspan (maybe a loop that computes a size_t length of sorts), the frame_entries could just be all_entries.subspan(length)?
Related details
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 10 comments![]( "Open in Gerrit")
Okay I think this all makes sense to me. Sending some minor feedback, I want to read through one more tomorrow with fresh eyes before I +1.
DCHECK(entry);nit: DCHECK probably not needed since allocation failure will crash.
// TODO(crbug.com/378647854): Move this to happen before we request(Aside: this feels very brittle, since this races with `current_frame_index_` getting updated. Or maybe it isn't in practice since the promise is likely to run before the next input?).
current_frame_index_++;I'm a bit surprised this happens in this method. Doesn't this mean that only when there are event timings without next paint (this method is invoked conditionally) will the thingy get incremented?
It would be more expected to have this increment in the caller of this method?
I see there's already another one in OnPaintFinished.
I think to hit this code it requires getting a presentation time callback _without_ a paint, in which case we set a more appropriate timestamp. And since we've "handled" all of the event timing entries for the current "frame", we need to increment this to make those eligible to be emitted (plus we're done with that group)?
This is probably worth a comment, but IIUC it makes sense/is needed.
event_timing_entries_.clear();Just because we might be in the middle of a stack with multiple event timing processing, it might be best not to touch this vector.
We don't call ReportEventTiming() from processing in this patch, but there is an upcoming change that will add that...
Can you remind me -- why does the CHECK no longer apply?
while (!event_timing_entries_.empty()) {Not for this patch, but I wonder if this things would be a bit cleaner with a different data structure? Either a HeapDeque<> if we're always removing from the front and appending to the back, or maybe better yet a list of Vectors, one per each frame (or a list of PerFrameEventTimingData, each of which contains a list)?
UNSAFE_BUFFERS(base::span(all_entries.begin(), end_of_frame_it));I wonder if this could be done with subspan instead, without requiring the allcaps UNSAFE.
If this std::ranges thingy in l. 940 could be reformulated to directly produce the length of the subspan (maybe a loop that computes a size_t length of sorts), the frame_entries could just be all_entries.subspan(length)?
+1. I don't know enough about spans, but IIUC this is meant to be temporary until usages are fixed: https://source.chromium.org/chromium/chromium/src/+/main:base/compiler_specific.h;l=1044-1047;drc=6d3e2ca997ca3ef995f719b22a1658a250a22d1e;bpv=1;bpt=1
// Unless ALL events in this range are ready to be reported, break out. //nit: Some stray slashes got in there
```suggestion
// Unless ALL events in this range are ready to be reported, break out.
```
// frame group is now eligible for reporting (even if it didn't request aIt took me a bit to understand why calling ReportEventTimings here is both okay and needed. IIUC in some cases events set a fallback time during processing (rare -- Mac only for artificial pointer ups [1], and that's why this is needed?). And also, if there are other events ahead of it in the queue waiting on presentation time, they'll have to wait until then.
I think maybe expanding the comment to indicate which event timings are expected to be reported would be helpful, since the current frame group being eligible here to actually be reported is expected to be rare (and was messing up my mental model!)?
Related details
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 5 comments![]( "Open in Gerrit")
uint64_t frame_index = 0;Is it worthwhile saying somewhere what is the smallest "real" frame index that we expect?
Yes, but, I also think its not worth investing too much time into extensive documentation because the goal is to replace manual tracking with PaintTimingMixin. We probably will have SOME frame_index still, just like the paint records, so I'll add a brief note.
uint64_t current_frame_index_ = 1;From this, I gather that the smallest 'real' frame index may be 1, is that right?
Correct-- I will update docs.
entry->GetEventTimingReportingInfo()->frame_index = current_frame_index_;Would it be possible to set frame_index on reporting_info prior to PerformanceEventTiming::Create?
If so, it seems that reporting_info is a local variable, so it could be more efficient to std::move it when ::Create is getting called actually - I think we're making a copy then, right now?
oh yeah, for sure we can, I love that suggestion, will change.
current_frame_index_++;I'm a bit surprised this happens in this method. Doesn't this mean that only when there are event timings without next paint (this method is invoked conditionally) will the thingy get incremented?
It would be more expected to have this increment in the caller of this method?
I see there's already another one in OnPaintFinished.
Yes, there are exactly two places where this increments (as you notice):
- OnPaintFinished
- Here, which is the "early end" of a "frame" that doesn't need next paint.
Conceptually this matches the LoAF model of "Task or Frame".
We only get to ReportEventTimingsWithoutNextPaint at the end of a bunch of event dispatch so not after every single event.
However, the specific hooks will change a bit to make this all less confusing as we adopt PaintTimingMixin and TaskTimeObserver (But I've put a hold on that integration until we finish this batch of cleanup)
UNSAFE_BUFFERS(base::span(all_entries.begin(), end_of_frame_it));I wonder if this could be done with subspan instead, without requiring the allcaps UNSAFE.
If this std::ranges thingy in l. 940 could be reformulated to directly produce the length of the subspan (maybe a loop that computes a size_t length of sorts), the frame_entries could just be all_entries.subspan(length)?
I did have a version that did that, but it just felt convoluted to basically create a range, find the end fo the range, count the entries in that sub-range, then count subspan.
Doing it that way "tricks" the compiler/linter from giving us a warning, but its JUST the same iterator operations + extra steps. I was weary of this big scary warning, but I saw a bunch of similar usage in the code.
Perhaps Scott has a suggestion.
Related details
- Johannes Henkel
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 13 comments![]( "Open in Gerrit")
Patch incoming, comments first.
Ty for amazing insights!
uint64_t frame_index = 0;Michal MocnyIs it worthwhile saying somewhere what is the smallest "real" frame index that we expect?
Yes, but, I also think its not worth investing too much time into extensive documentation because the goal is to replace manual tracking with PaintTimingMixin. We probably will have SOME frame_index still, just like the paint records, so I'll add a brief note.
Done
// Presentation promise index in which the entry in |event_timing_| was
// added.Is this comment out of date?
Done
uint64_t current_frame_index_ = 1;Michal MocnyFrom this, I gather that the smallest 'real' frame index may be 1, is that right?
Correct-- I will update docs.
Done
DCHECK(entry);nit: DCHECK probably not needed since allocation failure will crash.
Done
entry->GetEventTimingReportingInfo()->frame_index = current_frame_index_;Michal MocnyWould it be possible to set frame_index on reporting_info prior to PerformanceEventTiming::Create?
If so, it seems that reporting_info is a local variable, so it could be more efficient to std::move it when ::Create is getting called actually - I think we're making a copy then, right now?
oh yeah, for sure we can, I love that suggestion, will change.
I also did the same for is_processing_fully_nested_in_another_event, sweet.
// TODO(crbug.com/378647854): Move this to happen before we request(Aside: this feels very brittle, since this races with `current_frame_index_` getting updated. Or maybe it isn't in practice since the promise is likely to run before the next input?).
I think this doesn't race, but I agree that this is terrible (and gets removed with PaintTimingMixin).
At the [end of Event Dispatch](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/widget/input/widget_input_handler_manager.cc;l=485-491) we manually "break" the swap promise which synchronously calls the presentation time, and this logic is what we use to detect that this happens. We must have a different frame_index_ if we're "after paint', and must have the same index if we are still "before paint".
Older documentation here might be a good quick read? https://docs.google.com/document/d/1fsCbKdMwtqMObzILXKO6Us5v22W9RJloyBq_Qf0jc8Q
current_frame_index_++;Michal MocnyI'm a bit surprised this happens in this method. Doesn't this mean that only when there are event timings without next paint (this method is invoked conditionally) will the thingy get incremented?
It would be more expected to have this increment in the caller of this method?
I see there's already another one in OnPaintFinished.
Yes, there are exactly two places where this increments (as you notice):
- OnPaintFinished
- Here, which is the "early end" of a "frame" that doesn't need next paint.
Conceptually this matches the LoAF model of "Task or Frame".
We only get to ReportEventTimingsWithoutNextPaint at the end of a bunch of event dispatch so not after every single event.
However, the specific hooks will change a bit to make this all less confusing as we adopt PaintTimingMixin and TaskTimeObserver (But I've put a hold on that integration until we finish this batch of cleanup)
Done
event_timing_entries_.clear();Scott HaseleyJust because we might be in the middle of a stack with multiple event timing processing, it might be best not to touch this vector.
We don't call ReportEventTiming() from processing in this patch, but there is an upcoming change that will add that...
Can you remind me -- why does the CHECK no longer apply?
I think that in this patch it is fine, but in a future patch I call ReportEventTimings() from more places, including in the middle of event dispatch (for cases where we get a fallback earlier, such as js prompts or contextmenu events).
And, since you can detach window from an earlier event in the same task, this problem came up again.
I could defer this change to the CL that needs it, but i though this was a good safeguard and worth keeping together.
---
I can adjust to ensure all callers check this first, adn add the check there.
while (!event_timing_entries_.empty()) {Not for this patch, but I wonder if this things would be a bit cleaner with a different data structure? Either a HeapDeque<> if we're always removing from the front and appending to the back, or maybe better yet a list of Vectors, one per each frame (or a list of PerFrameEventTimingData, each of which contains a list)?
I think I tried HeapDeque but its not contiguous so it made it not possible to use spans for grame-group iterateion.
---
Yeah a queue of vectors, for each unique frame group, would be my preferred option-- but usually we dont have such a deep queue. Also maybe there is a way to consolidate this pattern with all the per-paint detectors...
But not for this patch!
// Unless ALL events in this range are ready to be reported, break out. //nit: Some stray slashes got in there
```suggestion
// Unless ALL events in this range are ready to be reported, break out.
```
Done
DCHECK(entry);Michal Mocnynit: CHECK?
Ive just gone ahead and updated all the DCHECKs related to Evernt Timing to CHECK.
There are a lot more DCHECKS in this file tho.
// frame group is now eligible for reporting (even if it didn't request aIt took me a bit to understand why calling ReportEventTimings here is both okay and needed. IIUC in some cases events set a fallback time during processing (rare -- Mac only for artificial pointer ups [1], and that's why this is needed?). And also, if there are other events ahead of it in the queue waiting on presentation time, they'll have to wait until then.
I think maybe expanding the comment to indicate which event timings are expected to be reported would be helpful, since the current frame group being eligible here to actually be reported is expected to be rare (and was messing up my mental model!)?
This didn't used to flush and I think I can remove this change here now.
One new fallback was added recently `FallbackReason::kWindowDestroyed` and when that happens we exit early without requesting presetation, and probably this should end the current frame and report event timings--
But right now the layering is weird: ProcessingEnd may still be in the middle of a task with more events on the stack, so I dont want to touch the frame_index there, and also I dont want to start reporting these right there.
But also in that case we almost certainly wont paint again?
---
I think the answer is to leave it as is, potentially fix when adopting TaskTimeObserver.
---
Also, in an upcominng patch, I change when we check and assign Fallback times to be more eager so we dont request presentation time when we know we dont need one just to override it. When I do that, I also flush timings-- though I might need to rethink some of that in light of the above insights...
For now-- removing this as not really needed.
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
UNSAFE_BUFFERS(base::span(all_entries.begin(), end_of_frame_it));I wonder if this could be done with subspan instead, without requiring the allcaps UNSAFE.
If this std::ranges thingy in l. 940 could be reformulated to directly produce the length of the subspan (maybe a loop that computes a size_t length of sorts), the frame_entries could just be all_entries.subspan(length)?
I did have a version that did that, but it just felt convoluted to basically create a range, find the end fo the range, count the entries in that sub-range, then count subspan.
Doing it that way "tricks" the compiler/linter from giving us a warning, but its JUST the same iterator operations + extra steps. I was weary of this big scary warning, but I saw a bunch of similar usage in the code.
Perhaps Scott has a suggestion.
You were right... you were right :)
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
Johannes Henkel (Gerrit)
Johannes Henkel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Code-Review | +1 |
2 comments
Looks sweet!
Feel free to skip the trivial typo there.
// |frane_index| in which the entry in |event_timing_| was first created.typo: frame_index
Related details
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 2 comments![]( "Open in Gerrit")
(resolving the typo comment to save another round of uploading and cq, but waiting for scott to sign off). Can fix typo in subsequent cl.
// |frane_index| in which the entry in |event_timing_| was first created.- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 1 comment![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[event-timing] Improve grouping by frame index and fix iteration.
This patch refactors how EventTiming groups events for reporting by
replacing 'presentation_index' with a 'frame_index'. The primary
difference is that all events are immediately assigned to a frame group
at processing start, rather than after processing end. Events will now
always group into frames, even without requests for presentation time.
And, we can more carefully and conditionally request presentation time
only when we need it.
This new model is a step towards PaintTimingMixin and prepares for some
more feature work that benefits from easier per-animation-frame
interaction.
Key improvements:
- Assign frame_index immediately and consistently.
- Decouple Presentation Requests: Limits presentation time requests to only when an event actually requires visual feedback.
- Safer Buffer Management: Refactored reporting loops to use base::span and indices, eliminating unsafe raw iterator arithmetic.
- Robust Metric Calculation: Fixes a pre-existing bug where TotalNonOverlappingProcessingDuration was calculated using all pending events instead of just the current frame's events.
- Hardened Lifecycle: Added validation checks to ensure presentation time is only assigned to events that completed the rendering pipeline.
- M third_party/blink/renderer/core/timing/performance_event_timing.h
- M third_party/blink/renderer/core/timing/window_performance.cc
- M third_party/blink/renderer/core/timing/window_performance.h
- M third_party/blink/renderer/core/timing/window_performance_test.cc
Code-Review: +1 by Johannes Henkel, +1 by Scott Haseley
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |