[soft navs] Refactor pending ICP entry buffering [chromium/src : main]
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
PTAL. The next couple of CLs in the chain shows where this is heading. Emitting more entries is small change on top of the last CL, which I just need to add tests for.
Related details
- Michal Mocny
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 and added 12 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
12 comments
where we either create and emit it synchronously or buffer it for later.I like that. Also, I think post-OT we want to change to not hold back ICP entries, anyway.
calculator. We don't plan to expose these values, as the recordsNit: technically this is requested of us, and we might in theory.
But, I still think the LCP calculator could consider the pair of largest image and text as a single candidate that it emits together, if that happens.
// if there is one. The context must not have been previously emitted.Could we Emit a soft-nav without an ICP? (I guess yes, until we "harden"?)
// few frames/paints. This is potentially unlikely given the low paint area
// requirement right now, but increasingly likely as we bump that up. We doNit: maybe just remove this part of the comment now?
if (RuntimeEnabledFeatures::SoftNavigationHeuristicsEnabled(window_)) {Hmmm, we guard the actual emit further down, anyway:
And along the way we emit trace events. I wonder if we should just call this unconditionally?
performance->AddSoftNavigationEntry(Since we are moving PerfEntry creation into this class for the ICP entry, I wonder if we should do the same for Soft-Nav (and hard-LCP?)
Looking at WindowPerformance, most apis do just pass data and let it get created there, often lazily based on buffer, but many of the recent apis create the entry first and pass it along...
(I have no preference, just wonder if we are duplicating constructor signature basically anyway)
// TODO(crbug.com/448974465): We currently don't emit ICP entries or record
// metrics for soft navs that are interrupted by a new interaction when there
// is pending presentation feedback for FCP, but we do emit the soft nav
// entry. We might want to reconsider this.I guess this affects more than just Paint->Presentation, it also could be:
- Interaction
- Network, Routing, Dom update, Paint...
- ...
- URL (finally)
It's more common to set URL before or during routing, and increasingly so, but some sites still do it very late/lazy after render.
(And Shopify team recently shared that they sometimes actually yield() after router rendering and before updating URL, on purpose, because it prevents unwanted scripts from triggering...)
---
Do you think we could just remove this restriction? We wouldn't accept new paint candidates after input, anyway?
(Fine to decouple from this patch, just curious.)
void SoftNavigationContext::EmitPerformanceEntry(In hindsight I wonder if this should have had "LCP" in its name-- since there are 2 types of perf entry this class manages, its ambiguous without knowing which base class we override...
CHECK(RuntimeEnabledFeatures::SoftNavigationHeuristicsEnabled(window_));Similarly, we could leave this unenforced.
Obviously we don't need to create entries if we aren't going to expose them, which is what the flag guards-- but now we are sort of using the entry as a temporary buffer which is useful also for tracing.
OnMetricsChanged(context);Nit: I think this this is specifically about soft-LCP and might be worth keeping in the name?
if (context != context_for_current_url_) {I think today this could be a CHECK today? But might change if we start to emit ICP always, as the TODO in `UpdateSoftLcpCandidate` suggests?
In which case, the name of `context_for_current_url_` should really be more like "current_active_navigation_context_" or something?
void OnInteractionContentfulPaintUpdated(InteractionContentfulPaint*);I'm fine with this, but FYI `AddLongAnimationFrameEntry` just uses the base class PerformanceEntry* to avoid the type declaration.
Related details
- 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 7 comments![]( "Open in Gerrit")
// if there is one. The context must not have been previously emitted.Could we Emit a soft-nav without an ICP? (I guess yes, until we "harden"?)
The code coverage lines indicate the early-exit is being hit, but I'm not entirely sure how. I think the only way we wouldn't is if we consider something an FCP but not an LCP, which I don't _think_ can happen? I had considered making this a CHECK instead, but thought I maybe had a scenario that would trip this. I'll poke at this a bit.
// few frames/paints. This is potentially unlikely given the low paint area
// requirement right now, but increasingly likely as we bump that up. We doNit: maybe just remove this part of the comment now?
SGTM.
if (RuntimeEnabledFeatures::SoftNavigationHeuristicsEnabled(window_)) {Hmmm, we guard the actual emit further down, anyway:
And along the way we emit trace events. I wonder if we should just call this unconditionally?
We currently flag-guard this in SNH: https://source.chromium.org/chromium/chromium/src/+/b068c0e68af54aee01cc40a55f50a7be0f1f22e7:third_party/blink/renderer/core/timing/soft_navigation_heuristics.cc;l=487-491, with the comment "Performance timeline won't allow emitting soft-LCP entries without this flag, but we can save some needless work by just not even trying to report." So that other check is redundant, and this matches the current behavior.
FWIW I like the guard here so more of the feature is behind the flag. Hard navs emits trace events for metrics candidates as well; maybe we should consider doing that for soft navs? Although, not sure having both makes sense for either case.
performance->AddSoftNavigationEntry(Since we are moving PerfEntry creation into this class for the ICP entry, I wonder if we should do the same for Soft-Nav (and hard-LCP?)
Looking at WindowPerformance, most apis do just pass data and let it get created there, often lazily based on buffer, but many of the recent apis create the entry first and pass it along...
(I have no preference, just wonder if we are duplicating constructor signature basically anyway)
I was wondering that as well. Let's discuss offline and follow up? I'd rather keep this scoped to ICP, but you're right that we should discuss it (I actually might have a patch somewhere that does this for hard navs for some something unrelated..).
// TODO(crbug.com/448974465): We currently don't emit ICP entries or record
// metrics for soft navs that are interrupted by a new interaction when there
// is pending presentation feedback for FCP, but we do emit the soft nav
// entry. We might want to reconsider this.I guess this affects more than just Paint->Presentation, it also could be:
- Interaction
- Network, Routing, Dom update, Paint...
- ...
- URL (finally)
It's more common to set URL before or during routing, and increasingly so, but some sites still do it very late/lazy after render.
(And Shopify team recently shared that they sometimes actually yield() after router rendering and before updating URL, on purpose, because it prevents unwanted scripts from triggering...)
---
Do you think we could just remove this restriction? We wouldn't accept new paint candidates after input, anyway?
(Fine to decouple from this patch, just curious.)
What should happen if:
- Interaction
- Network, Routing, Dom update, Paint...
- ...
- Another interaction
- ...
- URL (finally)
Should we still emit if we've stopped counting its paints? I don't think I've totally wrapped my head around how to best handle concurrent interactions.
--
I _think_ just removing this would be okay --- there might be a test that this affects, but I can't remember.
void SoftNavigationContext::EmitPerformanceEntry(In hindsight I wonder if this should have had "LCP" in its name-- since there are 2 types of perf entry this class manages, its ambiguous without knowing which base class we override...
Yeah, let's update that in a follow-up. I also thought this was getting confusing while drafting this.
CHECK(RuntimeEnabledFeatures::SoftNavigationHeuristicsEnabled(window_));Similarly, we could leave this unenforced.
Obviously we don't need to create entries if we aren't going to expose them, which is what the flag guards-- but now we are sort of using the entry as a temporary buffer which is useful also for tracing.
See comment above -- we're not currently tracing this. I think I'd rather trace the metrics candidate (which needs less info) and have anything that touches an ICP entry behind the flag.
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. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
calculator. We don't plan to expose these values, as the recordsNit: technically this is requested of us, and we might in theory.
But, I still think the LCP calculator could consider the pair of largest image and text as a single candidate that it emits together, if that happens.
We don't have to expose the records necessarily, only enough information to meet use cases. Some of the info is just needed to get the timing, which could be decoupled from the timing itself.
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. |
Scott Haseley (Gerrit)
Scott Haseley added 5 comments![]( "Open in Gerrit")
I rebased this on the prefinalizer change, and it should be in better shape now. I left a few comments open for visibility.
// if there is one. The context must not have been previously emitted.Scott HaseleyCould we Emit a soft-nav without an ICP? (I guess yes, until we "harden"?)
The code coverage lines indicate the early-exit is being hit, but I'm not entirely sure how. I think the only way we wouldn't is if we consider something an FCP but not an LCP, which I don't _think_ can happen? I had considered making this a CHECK instead, but thought I maybe had a scenario that would trip this. I'll poke at this a bit.
OK so multiple things:
- we don't buffer without the web-exposed feature enabled, so it could have be null in that case.
- but also, there was a bug since we were only updating candidates for the context associated with the current URL. This wasn't caught by any tests, but crashed when I added a CHECK. So I fixed this and updated one of the tests to also check for the ICP entry.
// few frames/paints. This is potentially unlikely given the low paint area
// requirement right now, but increasingly likely as we bump that up. We doScott HaseleyNit: maybe just remove this part of the comment now?
Scott HaseleySGTM.
Done
OnMetricsChanged(context);Scott HaseleyNit: I think this this is specifically about soft-LCP and might be worth keeping in the name?
Changed to `UpdateSoftLcpMetricsForContext()`. The method now only updates metrics based on existing values (vs updating the candidate), so I wanted to change it to indicate that.
if (context != context_for_current_url_) {I think today this could be a CHECK today? But might change if we start to emit ICP always, as the TODO in `UpdateSoftLcpCandidate` suggests?
In which case, the name of `context_for_current_url_` should really be more like "current_active_navigation_context_" or something?
I think it's possible, however unlikely, to delay the soft-nav entry while waiting for FCP presentation time and have `context_for_current_url_` change in the meantime. We ordered the checks in `MaybeCommitNavigationOrEmitSoftNavigationEntry` to account for that. In that case, we'd emit the soft nav entry (metrics and perf timeline), but not update soft LCP metrics or emit the perf timeline entry (to your other comments). So I think this keeps the status quo, though I think we should rethink that as a follow-up.
---
Also, FYI: in crrev.com/c/7181848, which is 2 away, I'm for the metrics update call to be driven async just like the EmitPerformanceEntry method to streamline the LCP candidate update dance, in which case this will be needed.
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 voted and added 4 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
4 comments
if (RuntimeEnabledFeatures::SoftNavigationHeuristicsEnabled(window_)) {Scott HaseleyHmmm, we guard the actual emit further down, anyway:
And along the way we emit trace events. I wonder if we should just call this unconditionally?
We currently flag-guard this in SNH: https://source.chromium.org/chromium/chromium/src/+/b068c0e68af54aee01cc40a55f50a7be0f1f22e7:third_party/blink/renderer/core/timing/soft_navigation_heuristics.cc;l=487-491, with the comment "Performance timeline won't allow emitting soft-LCP entries without this flag, but we can save some needless work by just not even trying to report." So that other check is redundant, and this matches the current behavior.
FWIW I like the guard here so more of the feature is behind the flag. Hard navs emits trace events for metrics candidates as well; maybe we should consider doing that for soft navs? Although, not sure having both makes sense for either case.
Resolving because we do this-- but it also came up in Connor Clarks DevTools update that he is seeing soft-nav trace event but not soft-LCP and I wonder if it is just that he didnt have SNH flag enabled...
Scott HaseleySince we are moving PerfEntry creation into this class for the ICP entry, I wonder if we should do the same for Soft-Nav (and hard-LCP?)
Looking at WindowPerformance, most apis do just pass data and let it get created there, often lazily based on buffer, but many of the recent apis create the entry first and pass it along...
(I have no preference, just wonder if we are duplicating constructor signature basically anyway)
I was wondering that as well. Let's discuss offline and follow up? I'd rather keep this scoped to ICP, but you're right that we should discuss it (I actually might have a patch somewhere that does this for hard navs for some something unrelated..).
Acknowledged
// TODO(crbug.com/448974465): We currently don't emit ICP entries or record
// metrics for soft navs that are interrupted by a new interaction when there
// is pending presentation feedback for FCP, but we do emit the soft nav
// entry. We might want to reconsider this.I guess this affects more than just Paint->Presentation, it also could be:
- Interaction
- Network, Routing, Dom update, Paint...
- ...
- URL (finally)
It's more common to set URL before or during routing, and increasingly so, but some sites still do it very late/lazy after render.
(And Shopify team recently shared that they sometimes actually yield() after router rendering and before updating URL, on purpose, because it prevents unwanted scripts from triggering...)
---
Do you think we could just remove this restriction? We wouldn't accept new paint candidates after input, anyway?
(Fine to decouple from this patch, just curious.)
What should happen if:
- Interaction
- Network, Routing, Dom update, Paint...
- ...
- Another interaction
- ...
- URL (finally)
Should we still emit if we've stopped counting its paints? I don't think I've totally wrapped my head around how to best handle concurrent interactions.--
I _think_ just removing this would be okay --- there might be a test that this affects, but I can't remember.
In the example you give, the timeline would be consistent in its reporting (perfectly non-overlapping paint entries), so its semantically equivalent to URL before Interaction but performance observer buffering for a long time.
Even if the second interaction painted.
---
I think even cases like this could be fine:
- Interaction 1 paints
- Interaction 2 paints
- Interaction 2 updates URL
- Interaction 1 updates URL
... the only problem with this case is that interaction 1 would no longer be reporting new ICP's.
---
Anyway-- I think erring on reporting rather than dropping is better here, but I'm leaving comment resolved so we can fiddle with it after this patch.
CHECK(RuntimeEnabledFeatures::SoftNavigationHeuristicsEnabled(window_));Scott HaseleySimilarly, we could leave this unenforced.
Obviously we don't need to create entries if we aren't going to expose them, which is what the flag guards-- but now we are sort of using the entry as a temporary buffer which is useful also for tracing.
See comment above -- we're not currently tracing this. I think I'd rather trace the metrics candidate (which needs less info) and have anything that touches an ICP entry behind the flag.
The point might become moot anyway once we emit ICP without buffering, so resolving.
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/56743.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- 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 and added 3 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
3 comments
// for the text. Moving more logic into presentation time, like we do for
// hard LCP, in conjunction with emitting largest presented image/text
// (vs. pending image) would fix this.I guess the main reason for doing it at paint time is to increment navigationID so that other entries are segmented accordingly. Otherwise the timing of the navigationID change is based on arbitrary scheduling of presentation feedback.
But with the intended ICP change to report before soft-nav, we no longer need navigationId for the ICP timings themselves... and potentially we could just using timestamps for other entries.
(I speculated about dropping navigationID earlier this year... let's see)
TRACE_EVENT_INSTANT("scheduler,devtools.timeline,loading",Maybe move this below the flag check now? Or do you want to do that in a separate patch?
Connor mentioned NOT guarding trace events on flag settings, but I think we decided to start with consistency first and then moving all the relevant trace events afterwards?
Related details
- 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 voted and added 3 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Commit-Queue | +2 |
3 comments
Thanks!
// for the text. Moving more logic into presentation time, like we do for
// hard LCP, in conjunction with emitting largest presented image/text
// (vs. pending image) would fix this.I guess the main reason for doing it at paint time is to increment navigationID so that other entries are segmented accordingly. Otherwise the timing of the navigationID change is based on arbitrary scheduling of presentation feedback.
But with the intended ICP change to report before soft-nav, we no longer need navigationId for the ICP timings themselves... and potentially we could just using timestamps for other entries.
(I speculated about dropping navigationID earlier this year... let's see)
Good point. We also track the total area, so we need all paints currently. I think we could still support since we already have decoupled incrementing nav id with presentation feedback (i.e. we wait for FCP presentation time) -- it's the candidate update that's a problem, and that could be moved around, which I _think_ will be straightforward once LCP calculator owns the records.
TRACE_EVENT_INSTANT("scheduler,devtools.timeline,loading",Maybe move this below the flag check now? Or do you want to do that in a separate patch?
Connor mentioned NOT guarding trace events on flag settings, but I think we decided to start with consistency first and then moving all the relevant trace events afterwards?
Lets do that separately and attribute it to the tracing bug. I'll send a follow-up once this lands.
Related details
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
[soft navs] Refactor pending ICP entry buffering
Currently, SoftNavigationContext tracks the current largest text and
image records, and uses those to emit the ICP performance entry and
update metrics if and when the soft navigation entry is emitted. This CL
changes this flow so that now we track the latest un-emitted
InteractionContentfulPaint object and emit that directly. To do this,
the LCP calculator continuously updates its state and calls the Delegate
EmitPerformanceEntry method, but actual ICP emission is guarded there,
where we either create and emit it synchronously or buffer it for later.
This is being done because:
1. To unify ICP and LCP and to better control lifetime of
PaintTimingRecords (in advance of strongifying some of its fields),
we want to move the largest text and image tracking into the LCP
calculator. We don't plan to expose these values, as the records
themselves shouldn't be persisted beyond getting presentation
feedback, but SNC needs to buffer the latest candidate indefinitely.
2. We plan to guarantee the Node and MediaObject are alive through this
callback, which might not be the case now if the node is removed,
and capturing the state here ensures we have what we need for
emission.
- M third_party/blink/renderer/core/timing/soft_navigation_context.cc
- M third_party/blink/renderer/core/timing/soft_navigation_context.h
- M third_party/blink/renderer/core/timing/soft_navigation_heuristics.cc
- M third_party/blink/renderer/core/timing/soft_navigation_heuristics.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/web_tests/external/wpt/soft-navigation-heuristics/smoke/tentative/late-url-change.html
Code-Review: +1 by Michal Mocny
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/56743
Related details
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 has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |