[soft navs] Total order from SNH to PTMS::DidObserveSoftNavigation [chromium/src : main]
Johannes Henkel (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Annie Sullivan
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Annie Sullivan (Gerrit)
Annie Sullivan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Annie Sullivan
| Code-Review | +1 |
1 comment
CHECK_GT(new_metrics.navigation_id, soft_navigation_metrics_->navigation_id);Might be good to have a comment about why we expect the navigation_id to be greater.
Related details
- Johannes Henkel
- 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 1 comment![]( "Open in Gerrit")
CHECK_GT(new_metrics.navigation_id, soft_navigation_metrics_->navigation_id);I thought this ID can overflow, and that we handle that? Do we not expect that to happen in practice?
Related details
- Johannes Henkel
- 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. |
Johannes Henkel (Gerrit)
Johannes Henkel voted and added 3 comments![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Commit-Queue | +1 |
3 comments
CHECK_GT(new_metrics.navigation_id, soft_navigation_metrics_->navigation_id);I thought this ID can overflow, and that we handle that? Do we not expect that to happen in practice?
Yes you're right - good catch!
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/timing/navigation_id_generator.cc;l=37;drc=f4b1a5f91e14941349a0319b35f71d3efb7350af;bpv=1;bpt=1
The overflow is not likely (requires 280M+ soft navs) but maybe it's better to be principled also in case we change the generation at some point.
The count can overflow as well, but I think 4B+ soft navs within a hard nav ought to be enough for anybody. :-)
CHECK_GT(new_metrics.navigation_id, soft_navigation_metrics_->navigation_id);Might be good to have a comment about why we expect the navigation_id to be greater.
Good point. Now doing a CHECK_NE here (due to potential overflow), and added comment.
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 2 comments![]( "Open in Gerrit")
LG % test issues. I'll stamp after the bots are green.
CHECK_GT(new_metrics.navigation_id, soft_navigation_metrics_->navigation_id);Johannes HenkelI thought this ID can overflow, and that we handle that? Do we not expect that to happen in practice?
Yes you're right - good catch!
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/timing/navigation_id_generator.cc;l=37;drc=f4b1a5f91e14941349a0319b35f71d3efb7350af;bpv=1;bpt=1The overflow is not likely (requires 280M+ soft navs) but maybe it's better to be principled also in case we change the generation at some point.
The count can overflow as well, but I think 4B+ soft navs within a hard nav ought to be enough for anybody. :-)
Yeah unlikely in practice, but +1 to being consistent with the generator, since otherwise why have that. Checking NE is a nice compromise.
Related details
- Johannes Henkel
- Michal Mocny
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 1 comment![]( "Open in Gerrit")
// Now that we've checked the invariants, start with a fresh Mojom
// message, including a cleared out largest_contentful_paint field.
soft_navigation_metrics_ = CreateSoftNavigationMetrics();I'd like to try to make one more change here, while I'm at this. I'm concerned the previous version doesn't clear out largest_contentful_paint, which means that it could stick around from the previous soft nav. It would then actually get sent, if the delay from EnsureSendTimer is greater than the time it takes to collect the first LCP lcp belonging to this new soft nav.
Related details
- Annie Sullivan
- 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 2 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
2 comments
LGTM from my part with a small suggestion.
(I don't know if other reviewer concerns are fully addressed)
.count = ++soft_navigation_count_,Nit: Wonder if we make it clear that this value is "ForReporting" purposes only, at this point.
We have an api for "interactionCount" and if we ever wanted an api for "navigationCount" we wouldn't want to use this value as-is any more.
---
Alternatively, we currently report soft-nav to metrics from exactly 1 place: `SoftNavigationHeuristics::EmitSoftNavigationEntry`, which is also where we report to perf timeline.
So maybe just incrementing the count *there* makes more sense (even though it will be in the same sync task, the order makes more sense to me).
It also means you can make this function non-const again?
Related details
- Annie Sullivan
- 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. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 2 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
2 comments
lgtm also % comments
/*info=*/DOMPaintTimingInfo());nit/optional: would it be difficult to make this have real values? In the future, we might CHECK that these get set together.
Johannes Henkel (Gerrit)
Johannes Henkel voted and added 3 comments![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Commit-Queue | +2 |
3 comments
Thanks a lot.
Will try to submit./
Nit: Wonder if we make it clear that this value is "ForReporting" purposes only, at this point.
We have an api for "interactionCount" and if we ever wanted an api for "navigationCount" we wouldn't want to use this value as-is any more.
---
Alternatively, we currently report soft-nav to metrics from exactly 1 place: `SoftNavigationHeuristics::EmitSoftNavigationEntry`, which is also where we report to perf timeline.
So maybe just incrementing the count *there* makes more sense (even though it will be in the same sync task, the order makes more sense to me).
It also means you can make this function non-const again?
Yes, moved it to EmitSoftNavigationEntry with a comment and made this one const again. I guess we need to be careful to not refactor it out of sync though, that's why I tried putting it here. :-)
nit/optional: would it be difficult to make this have real values? In the future, we might CHECK that these get set together.
Not difficult - I added this just to try it and it continues to pass:
auto paint_time = base::TimeTicks::Now();
DOMHighResTimeStamp dom_timestamp =
DOMWindowPerformance::performance(
*GetDocument().domWindow())
->MonotonicTimeToDOMHighResTimeStamp(paint_time);
record->SetPaintTime(paint_time,
/*info=*/DOMPaintTimingInfo({dom_timestamp, dom_timestamp}));
But I think it's better to do that when adding the CHECK, not before, to see the effect of the increased checks in the same change. So will leave this as is for now.
Related details
- Annie Sullivan
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. |
Johannes Henkel (Gerrit)
Johannes Henkel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Commit-Queue | +2 |
1 comment
// Now that we've checked the invariants, start with a fresh Mojom
// message, including a cleared out largest_contentful_paint field.
soft_navigation_metrics_ = CreateSoftNavigationMetrics();I'd like to try to make one more change here, while I'm at this. I'm concerned the previous version doesn't clear out largest_contentful_paint, which means that it could stick around from the previous soft nav. It would then actually get sent, if the delay from EnsureSendTimer is greater than the time it takes to collect the first LCP lcp belonging to this new soft nav.
Done
Related details
- Annie Sullivan
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
9 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/timing/soft_navigation_heuristics.h
Insertions: 1, Deletions: 1.
@@ -143,7 +143,7 @@
base::FunctionRef<void(InteractionEffectsMonitor&)>);
private:
- void ReportSoftNavigationToMetrics(SoftNavigationContext*);
+ void ReportSoftNavigationToMetrics(SoftNavigationContext*) const;
void SetIsTrackingSoftNavigationHeuristicsOnDocument(bool value) const;
// We can grab a context from the "running task", or sometimes from other
```
```
The name of the file: third_party/blink/renderer/core/timing/soft_navigation_heuristics.cc
Insertions: 6, Deletions: 2.
@@ -407,6 +407,10 @@
CHECK(context->HasFirstContentfulPaint());
CHECK(!context->WasEmitted());
context->MarkEmitted();
+ // Since this is used for metrics reporting and sent as part of the
+ // SoftNavigationMetrics record, we must increment it before calling
+ // ReportSoftNavigationToMetrics.
+ soft_navigation_count_++;
WindowPerformance* performance = DOMWindowPerformance::performance(*window_);
CHECK(performance);
@@ -501,7 +505,7 @@
}
void SoftNavigationHeuristics::ReportSoftNavigationToMetrics(
- SoftNavigationContext* context) {
+ SoftNavigationContext* context) const {
LocalFrame* frame = window_->GetFrame();
// We should not be running paint timing callbacks for detached frames.
CHECK(frame);
@@ -513,7 +517,7 @@
if (LocalFrameClient* frame_client = frame->Client()) {
blink::SoftNavigationMetricsForReporting metrics = {
- .count = ++soft_navigation_count_,
+ .count = soft_navigation_count_,
.start_time = loader->GetTiming().MonotonicTimeToPseudoWallTime(
context->TimeOrigin()),
.first_contentful_paint =
```
Change information
[soft navs] Total order from SNH to PTMS::DidObserveSoftNavigation
Objective is to enforce with CHECKs that:
* The count of soft navigations increases strictly monotonically
* We never send a count of 0
* navigation_id changes for each soft nav, we never send
an absent navigation_id
* There's always a same_document_metrics_token, and it changes
with each update.
The main logic change is to pre-increment SNH::soft_navigation_count_
exactly when we decide to send the soft nav to the browser. There is also a small change in page_timing_metrics_sender to reset the soft_navigation_metrics_ to an empty record to avoid picking up a previous soft LCP.
- M components/page_load_metrics/renderer/page_timing_metrics_sender.cc
- 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/soft_navigation_heuristics_test.cc
Code-Review: +1 by Michal Mocny, +1 by Scott Haseley
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |