From 4bbdfd2ff5e273a08ae9d2ac78382c9cc57ea499 Mon Sep 17 00:00:00 2001 From: Mathijs Affourtit Date: Thu, 27 Apr 2023 22:48:20 +0000 Subject: [PATCH] Revert "Remove Blink.MemoryCache.RevalidationPolicy.PerTopFrameSite histogram." This reverts commit 804d774de51f646e4377a9ce89f37b6dcc873b01. Reason for revert: https://bugs.chromium.org/p/chromium/issues/detail?id=1440580 Issue 1440580: Chrome: Crash Report - blink::ChildURLLoaderFactoryBundle::CreateLoaderAndStart Original change's description: > Remove Blink.MemoryCache.RevalidationPolicy.PerTopFrameSite histogram. > > Remove the entry from histograms.xml, the metric logging in resource_fetcher.cc and associated tests in resource_fetcher_test.cc. > > > Bug: 1385063 > Change-Id: Ic94e11465b22d923a4d44289cb71fb60b888019b > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4460611 > Reviewed-by: Takashi Toyoshima > Commit-Queue: Mathijs Affourtit > Reviewed-by: Kinuko Yasuda > Code-Coverage: Findit > Cr-Commit-Position: refs/heads/main@{#1135168} Bug: 1385063 Change-Id: Ib4b0fea722ea6ca253fc4078266e29acdffd3ec6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4484559 Owners-Override: Srinivas Sista Reviewed-by: Harry Souders Commit-Queue: Srinivas Sista Cr-Commit-Position: refs/branch-heads/5735@{#38} Cr-Branched-From: 2f562e4ddbaf79a3f3cb338b4d1bd4398d49eb67-refs/heads/main@{#1135570} --- .../platform/loader/fetch/resource_fetcher.cc | 5 +++++ .../loader/fetch/resource_fetcher_test.cc | 20 +++++++++++++++++-- .../histograms/metadata/blink/histograms.xml | 13 ++++++++++++ 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc b/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc index 24472755f6c623..123c6ef7166b79 100644 --- a/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc +++ b/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc @@ -928,6 +928,11 @@ void ResourceFetcher::UpdateMemoryCacheStats( RecordResourceHistogram("Preload.", factory.GetType(), policy); } else { RecordResourceHistogram("", factory.GetType(), policy); + + // Log metrics to evaluate effectiveness of the memory cache if it was + // partitioned by the top-frame site. + if (same_top_frame_site_resource_cached) + RecordResourceHistogram("PerTopFrameSite.", factory.GetType(), policy); } // Aims to count Resource only referenced from MemoryCache (i.e. what would be diff --git a/third_party/blink/renderer/platform/loader/fetch/resource_fetcher_test.cc b/third_party/blink/renderer/platform/loader/fetch/resource_fetcher_test.cc index d5993c04a569dd..29183ff9c72e44 100644 --- a/third_party/blink/renderer/platform/loader/fetch/resource_fetcher_test.cc +++ b/third_party/blink/renderer/platform/loader/fetch/resource_fetcher_test.cc @@ -386,6 +386,9 @@ TEST_F(ResourceFetcherTest, MetricsPerTopFrameSite) { EXPECT_EQ(resource_1, resource_2); // Test histograms. + histogram_tester.ExpectTotalCount( + "Blink.MemoryCache.RevalidationPolicy.PerTopFrameSite.Mock", 0); + histogram_tester.ExpectTotalCount("Blink.MemoryCache.RevalidationPolicy.Mock", 2); @@ -397,7 +400,7 @@ TEST_F(ResourceFetcherTest, MetricsPerTopFrameSite) { 0 /* RevalidationPolicy::kUse */, 1); // Now load the same resource with origin_b as top-frame site. The - // histograms should be incremented. + // PerTopFrameSite histogram should be incremented. auto* fetcher_3 = CreateFetcher(); ResourceRequestHead request_head_3(url); scoped_refptr foo_origin_b = @@ -409,6 +412,11 @@ TEST_F(ResourceFetcherTest, MetricsPerTopFrameSite) { Resource* resource_3 = MockResource::Fetch(fetch_params_2, fetcher_3, nullptr); EXPECT_EQ(resource_1, resource_3); + histogram_tester.ExpectTotalCount( + "Blink.MemoryCache.RevalidationPolicy.PerTopFrameSite.Mock", 1); + histogram_tester.ExpectBucketCount( + "Blink.MemoryCache.RevalidationPolicy.PerTopFrameSite.Mock", + 0 /* RevalidationPolicy::kUse */, 1); histogram_tester.ExpectTotalCount("Blink.MemoryCache.RevalidationPolicy.Mock", 3); histogram_tester.ExpectBucketCount( @@ -457,6 +465,9 @@ TEST_F(ResourceFetcherTest, MetricsPerTopFrameSiteOpaqueOrigins) { EXPECT_EQ(resource_1, resource_2); // Test histograms. + histogram_tester.ExpectTotalCount( + "Blink.MemoryCache.RevalidationPolicy.PerTopFrameSite.Mock", 0); + histogram_tester.ExpectTotalCount("Blink.MemoryCache.RevalidationPolicy.Mock", 2); @@ -468,7 +479,7 @@ TEST_F(ResourceFetcherTest, MetricsPerTopFrameSiteOpaqueOrigins) { 0 /* RevalidationPolicy::kUse */, 1); // Now load the same resource with opaque_origin1 as top-frame site. The - // histograms should be incremented. + // PerTopFrameSite histogram should be incremented. auto* fetcher_3 = CreateFetcher(); ResourceRequestHead request_head_3(url); request_head_3.SetTopFrameOrigin(opaque_origin2); @@ -478,6 +489,11 @@ TEST_F(ResourceFetcherTest, MetricsPerTopFrameSiteOpaqueOrigins) { Resource* resource_3 = MockResource::Fetch(fetch_params_2, fetcher_3, nullptr); EXPECT_EQ(resource_1, resource_3); + histogram_tester.ExpectTotalCount( + "Blink.MemoryCache.RevalidationPolicy.PerTopFrameSite.Mock", 1); + histogram_tester.ExpectBucketCount( + "Blink.MemoryCache.RevalidationPolicy.PerTopFrameSite.Mock", + 0 /* RevalidationPolicy::kUse */, 1); histogram_tester.ExpectTotalCount("Blink.MemoryCache.RevalidationPolicy.Mock", 3); histogram_tester.ExpectBucketCount( diff --git a/tools/metrics/histograms/metadata/blink/histograms.xml b/tools/metrics/histograms/metadata/blink/histograms.xml index 62d83a52565c8f..a861b27a0ca2f4 100644 --- a/tools/metrics/histograms/metadata/blink/histograms.xml +++ b/tools/metrics/histograms/metadata/blink/histograms.xml @@ -2457,6 +2457,19 @@ chromium-metrics-reviews@google.com. + + shivanisha@chromium.org + privacy-sandbox-dev@chromium.org + + RevalidationPolicy used for requests for each resource type. Logged only if + the resource is found in the memory cache and if the same top-frame site had + loaded this resource earlier. + + + + hiroshige@chromium.org