[soft navs] Mojom Refactor (Update,Commit,FCP) and FCP in UKM [chromium/src : main]
Johannes Henkel (Gerrit)
Johannes Henkel added 4 comments![]( "Open in Gerrit")
duration: record.duration,'duration' is the thingy from the paint timing mixin, and it's the delta between the startTime of the navigation and the presentation time - so it's exactly what we want for comparing with the UKM logged FCP.
std::vector<double> soft_nav_start_times;
for (const auto& [source_id, start_time] : source_id_to_start_time) {
soft_nav_start_times.push_back(start_time);
}I moved this part up here (without changing anything) to make it easier to read (since this section is about getting the start times).
// Soft navigation FCP.
auto source_id_to_fcp = GetSoftNavigationMetrics(
ukm_recorder(),
SoftNavigation::kPaintTiming_FirstContentfulPaintMsName);
EXPECT_EQ(source_id_to_fcp.size(), expected_soft_nav_count);
std::vector<double> soft_nav_fcp;
for (const auto& [source_id, fcp] : source_id_to_fcp) {
soft_nav_fcp.push_back(fcp);
}This is new, it extracts the soft fcp that is recorded for UKM.
EXPECT_NEAR(*timing.FindDoubleByDottedPath("softNavigation.duration"),
soft_nav_fcp[i], 6);Here we compare the UKM recorded FCP with the paint timing mixin's duration.
Related details
- Annie Sullivan
- 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 24 comments![]( "Open in Gerrit")
duration: record.duration,extracting the duration from the performance timeline API, so we can compare it below (with the soft FCP)
std::vector<double> soft_nav_start_times;
for (const auto& [source_id, start_time] : source_id_to_start_time) {
soft_nav_start_times.push_back(start_time);
}
moved this up a bit, for readability
// Soft navigation FCP.
auto source_id_to_fcp = GetSoftNavigationMetrics(
ukm_recorder(),
SoftNavigation::kPaintTiming_FirstContentfulPaintMsName);
EXPECT_EQ(source_id_to_fcp.size(), expected_soft_nav_count);
std::vector<double> soft_nav_fcp;
for (const auto& [source_id, fcp] : source_id_to_fcp) {
soft_nav_fcp.push_back(fcp);
}We get the UKM recorded soft FCP and check that each softnav has one.
EXPECT_NEAR(*timing.FindDoubleByDottedPath("softNavigation.duration"),
soft_nav_fcp[i], 6);This checks that the Soft FCP in UKM and the duration on the performance timeline match.
tester()->SimulateSoftNavigation(&navigation_handle);This interface was unnecessary / weird - all we're doing here is committing a same document navigation; so now I'm using SimulateDidFinishNavigation instead, and I've removed SimulateSoftNavigation.
// Now simulate one SoftNavigationUpdate.
std::vector<page_load_metrics::mojom::SoftNavigationUpdatePtr>
soft_navigation_updates;
soft_navigation_updates.emplace_back(
page_load_metrics::mojom::SoftNavigationUpdate::New(
/*soft_navigation_offset=*/1,
page_load_metrics::mojom::SoftNavigationUpdatePayload::NewCommit(
page_load_metrics::mojom::SoftNavigationCommit::New(
/*start_time=*/base::Milliseconds(12),
/*soft_navigation_slicing_time=*/base::TimeTicks() +
base::Milliseconds(12),
/*navigation_type=*/
blink::mojom::NavigationTypeForNavigationApi::kPush,
/*same_document_metrics_token=*/
base::UnguessableToken::Create()))));
tester()->SimulateSoftNavigationUpdates(soft_navigation_updates);This interface is modernized in that we send soft navigation updates. A single update (no FCP) is needed to be counted as a soft nav.
mojom::SoftNavigationMetricsPtr soft_navigation_metrics_;Added this field just so what's returned by GetSoftNavigationMetrics() is owned by something.
return *mojom::SoftNavigationMetrics::New();I figured it's better to stick this into a field, it seems a bit sketch to dereference the temporary...
std::vector<mojom::SoftNavigationUpdatePtr> soft_navigation_updates,
std::vector<mojom::LargestContentfulPaintTimingPtr>
soft_largest_contentful_paint,So in this change, on the browser side, all the UpdateTiming and equivalent calls have a vector of SoftNavigationUpdate and a vector of soft LCP. In a later cl, I've put the soft LCP also into the updates, which has the advantage of being more efficient and retaining the order of insertion.
So if you don't see the point of sending updates, I think looking at that follow-up cl shows it:
https://chromium-review.git.corp.google.com/c/chromium/src/+/8029244/8/components/page_load_metrics/browser/metrics_web_contents_observer.h (and other files in that cl).
void SimulateSoftNavigationUpdates(
const std::vector<mojom::SoftNavigationUpdatePtr>& updates);Changed this to updates, it's more honest with the refactoring; but this also means the test code changes a bit (see ukm_page_load_metrics_observer_unittest.cc).
void SimulateSoftNavigation(content::NavigationHandle* navigation_handle);This method delegates to SimulateDidFinishNavigation anyway, so I got rid of it. The point is to commit a same-document navigation, which is how the ukm_page_load_metric_observer.cc gets the ukm source id for the soft nav.
std::vector<mojom::EventTimingPtr> event_timings_clone;
for (const auto& entry : event_timings) {
event_timings_clone.push_back(entry.Clone());
}This can just use mojo::Clone so I did that while I'm in here.
void SoftNavigationPageLoadMetricsObserver::RecordSoftFcp(
ukm::builders::SoftNavigation& builder,
const SoftNavigationMetrics& metrics,
const SoftNavigationCommit& commit) {
if (metrics.first_contentful_paint.has_value() &&
FromForegroundOptionalEventInForeground(
metrics.first_contentful_paint.value())) {
base::TimeDelta soft_fcp =
(metrics.first_contentful_paint.value() - commit.start_time);
builder.SetPaintTiming_FirstContentfulPaintMs(soft_fcp.InMilliseconds());
}
}Here we now record soft FCP to UKM, on a best-effort basis.
The logic to delay soft navs processing until presentation time (FCP) is received is in a subsequent cl, and there it's mostly in soft_navigation_tracker.cc.
CHECK(!soft_navigation_tracker_.HasNextSoftNavigation());
if (!soft_navigation_tracker_.ValidateAndProcessUpdates(
std::move(soft_navigation_updates))) {
return;
}
while (true) {
soft_navigation_tracker_.Process(
&event_timings, &soft_navigation_interaction_to_next_paint_);
soft_navigation_tracker_.Process(
&layout_shifts, &soft_navigation_layout_shift_normalization_);
size_t num_soft_lcps_processed = soft_navigation_tracker_.Process(
&soft_lcps, &soft_navigation_largest_contentful_paint_);
if (num_soft_lcps_processed) {
client_->OnSoftNavigationLargestContentfulPaint(num_soft_lcps_processed);
}
if (!soft_navigation_tracker_.HasNextSoftNavigation()) {
break;
}
client_->OnSoftNavigation(); // Notify observers, via PageLoadTracker.
soft_navigation_tracker_.AdvanceToNextSoftNavigation();
soft_navigation_interaction_to_next_paint_.ClearEventTimings();
soft_navigation_layout_shift_normalization_.ClearAllLayoutShifts();
soft_navigation_largest_contentful_paint_.Clear();
}Note how the processing loop hasn't changed significantly here, while we process stuff in order in which the soft navs are received, all data that came across the UpdateTiming is processed within the body of this method and soft navs are not kept between calls - even if the FCP for some soft nav is missing; see the DCHECK at the beginning of this section.
Only in a subsequent change is this loop substantially modified so that the soft navigation tracker and the calculators keep state between calls.
bool SoftNavigationTracker::ValidateAndProcessUpdates(The scheme for this validation is: Make a copy of the state, add the new stuff in the copied queue, if we see a problem bail (leaving behind the object unmodified), if it's good swap the modified state in.
switch (update->payload->which()) {This is one of the things I really like about the union, it ensures all cases are covered.
// Soft navigation metrics (see SoftNavigationMetrics) are sent as
// SoftNavigationUpdate from the renderer to the browser as soon as they are
// available; therefore, we tag the pieces with the `soft_navigation_offset`,
// and the payload is the union of the individual components.
struct SoftNavigationUpdate {
// The one-based offset of the interaction in time-based order; 1 for the
// first interaction, 2 for the second, etc.
uint64 soft_navigation_offset;
SoftNavigationUpdatePayload payload;
};
This is the main innovation in this change, when things get sent from renderer to browser, it's sent as SoftNavigationUpdate, and a union (the payload) identifies what type of update.
Though it's lame to just have two types of updates, the later cl allows the soft LCP as well, reducing the number of arguments to UpdateTiming.
for (auto& update : soft_navigation_updates) {
if (update->payload && update->payload->is_commit()) {
auto metrics = mojom::SoftNavigationMetrics::New();
metrics->soft_navigation_offset = update->soft_navigation_offset;
metrics->commit = std::move(update->payload->get_commit());
soft_navigation_metrics.push_back(std::move(metrics));
}
}OK, maybe I should plumb this into UpdateTiming as well, instead of converting back to SoftNavigationMetrics. Will look into it.
validator_.ExpectSoftNavigationMetrics(*soft_navigation_metrics);So yeah, maybe this should be expecting an update. It's a bit lame to have the SoftNavigationMetrics on the renderer side. Will look into this one.
CHECK(!commit.same_document_metrics_token.is_empty());
if (!soft_navigation_updates_.empty()) {
CHECK_EQ(soft_navigation_updates_.back()->soft_navigation_offset + 1,
commit.soft_navigation_offset);
if (soft_navigation_updates_.back()->payload->is_commit()) {
const auto& last_commit =
soft_navigation_updates_.back()->payload->get_commit();
CHECK_NE(commit.same_document_metrics_token,
last_commit->same_document_metrics_token);
}
}In a way, these checks are quite optional, since this is the renderer which we don't trust anyway.
What's important here is that the production code which enqueues the updates is *very* simple, it just constructs an update for the commit here, and one for the FCP below, and puts it into the soft_navigation_updates_ array.
In the later CL, same with soft LCP.
https://chromium-review.git.corp.google.com/c/chromium/src/+/8029244/8/components/page_load_metrics/renderer/page_timing_metrics_sender.cc
struct BLINK_EXPORT SoftNavigationCommitForReporting {Note while this metrics struct is now called "Commit", there is no struct for FCP. Reason being, FCP is just one value as payload, with a soft_navigation_offset to tag it, so we send these as two params directly.
virtual void DidObserveSoftNavigation(
const SoftNavigationCommitForReporting& commit) {}I made all these structs const refs as suggested by Scott in an earlier attempt of mine.
// TODO(crbug.com/490814752): Some tests simulate events with an impossibly
// small start_time value, which is less than the initial reference time,
// which makes the duration appear negative. We cannot report such values
// without failing expectations (in tests).
if (context->TimeOrigin() <= loader->GetTiming().ReferenceMonotonicTime()) {
return;
}I've started looking into this, but some tests are still failing if it's removed.
<metric name="PaintTiming.FirstContentfulPaintMs">
<summary>
Measures the FirstContentfulPaint in milliseconds from the start time of
the soft navigation to the presentation time.
</summary>
</metric>Adding this lets us update the google3 stuff asap. :-)
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. |
Johannes Henkel (Gerrit)
Johannes Henkel added 1 comment![]( "Open in Gerrit")
validator_.ExpectSoftNavigationMetrics(*soft_navigation_metrics);Johannes HenkelSo yeah, maybe this should be expecting an update. It's a bit lame to have the SoftNavigationMetrics on the renderer side. Will look into this one.
ok this is fixed (patchset 20/21)
Related details
- Annie Sullivan
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |