Skip to content

Commit

Permalink
Revert "Remove Blink.MemoryCache.RevalidationPolicy.PerTopFrameSite h…
Browse files Browse the repository at this point in the history
…istogram."

This reverts commit 804d774.

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 <toyoshim@chromium.org>
> Commit-Queue: Mathijs Affourtit <maffourtit@google.com>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
> 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 <srinivassista@chromium.org>
Reviewed-by: Harry Souders <harrysouders@google.com>
Commit-Queue: Srinivas Sista <srinivassista@chromium.org>
Cr-Commit-Position: refs/branch-heads/5735@{#38}
Cr-Branched-From: 2f562e4-refs/heads/main@{#1135570}
  • Loading branch information
Mathijs Affourtit authored and Chromium LUCI CQ committed Apr 27, 2023
1 parent 1a4f066 commit 4bbdfd2
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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<const SecurityOrigin> foo_origin_b =
Expand All @@ -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(
Expand Down Expand Up @@ -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);

Expand All @@ -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);
Expand All @@ -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(
Expand Down
13 changes: 13 additions & 0 deletions tools/metrics/histograms/metadata/blink/histograms.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2457,6 +2457,19 @@ chromium-metrics-reviews@google.com.
<token key="ResourceType" variants="ResourceType"/>
</histogram>

<histogram
name="Blink.MemoryCache.RevalidationPolicy.PerTopFrameSite.{ResourceType}"
enum="RevalidationPolicy" expires_after="2021-12-26">
<owner>shivanisha@chromium.org</owner>
<owner>privacy-sandbox-dev@chromium.org</owner>
<summary>
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.
</summary>
<token key="ResourceType" variants="ResourceType"/>
</histogram>

<histogram name="Blink.MemoryCache.RevalidationPolicy.Preload.{ResourceType}"
enum="RevalidationPolicy" expires_after="2023-10-01">
<owner>hiroshige@chromium.org</owner>
Expand Down

0 comments on commit 4bbdfd2

Please sign in to comment.