Skip to content

Commit

Permalink
Revert "Plumb resources serviced from memory cache to PLM"
Browse files Browse the repository at this point in the history
This reverts commit 44af1fd.

Reason for revert: DefaultKeyboardExtensionBrowserTest.LayoutTest is flaky on linux-chromeos-rel.

Bug: 979459

Original change's description:
> Plumb resources serviced from memory cache to PLM
> 
> Byte metrics in page load metrics should be indicative of all bytes loaded
> by the page. UMA shows that large numbers of resources (~45% of JS
> resources) come from the memory cache. To provide a better picture of
> everything loaded by the page, we should make memory cache resources
> visible to observers.
> 
> This change plumbs information about resources loaded via the blink
> memory cache into PageLoadMetrics. This is done though the existing
> SendTiming mojo, and these resources are included in the existing
> OnResourceDataUseObserved callback for PageLoadMetrics observers. This
> was added to the existing interface so that an observer that wants to
> look at all bytes loaded on the page does not need to implement two
> separate interfaces.
> 
> Because we modify the existing mojo, all existing observers need to be
> updated to either discard resources loaded by the memory cache, or need
> to have their relevant histograms versioned.
> 
> Observers whose metrics are updated:
>  - CorePageLoadMetricsObserver
>  - TabRestorePageLoadMetricsObserver
>  - MediaPageLoadMetricsObserver
>  - UkmPageLoadMetricsObserver
> 
> Observers whose metrics would be affected but are modified to ignore
> MemoryCache resources:
>  - ResourceMetricsObserver
>  - AdsPageLoadMetricsObserver
>  - DataReductionProxyMetricsObserver
> 
> Only the histograms versioned in this CL should change. For all histograms
> not versioned in this change, this should be a no-op.
> 
> Bug: 968141
> Change-Id: I67ed0977f8532616815ba0fbe6e1eb3f3b70e36f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1633190
> Commit-Queue: John Delaney <johnidel@chromium.org>
> Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
> Reviewed-by: Charlie Harrison <csharrison@chromium.org>
> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#672925}

TBR=kinuko@chromium.org,bmcquade@chromium.org,csharrison@chromium.org,johnidel@chromium.org

Change-Id: I6139ba2fea9b915cff2fdfb143216be14a3ea6e9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 968141
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1681887
Reviewed-by: Sergey Poromov <poromov@chromium.org>
Commit-Queue: Sergey Poromov <poromov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#673284}
  • Loading branch information
Sergey Poromov authored and Commit Bot committed Jun 28, 2019
1 parent 32283a3 commit c87d16d
Show file tree
Hide file tree
Showing 35 changed files with 70 additions and 364 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ class ResourceLoadingCancellingThrottle
resource->received_data_length = 10 * 1024;
resource->delta_bytes = 10 * 1024;
resource->encoded_body_length = 10 * 1024;
resource->cache_type = page_load_metrics::mojom::CacheType::kNotCached;
resource->was_fetched_via_cache = false;
resource->is_complete = true;
resource->is_primary_frame_resource = true;
resources.push_back(std::move(resource));
Expand Down Expand Up @@ -338,9 +338,7 @@ class AdsPageLoadMetricsObserverTest : public SubresourceFilterTestHarness {
resource->encoded_body_length = resource_size_in_kbyte << 10;
resource->reported_as_ad_resource = is_ad_resource;
resource->is_complete = true;
resource->cache_type = (resource_cached == ResourceCached::NOT_CACHED)
? page_load_metrics::mojom::CacheType::kNotCached
: page_load_metrics::mojom::CacheType::kHttp;
resource->was_fetched_via_cache = static_cast<bool>(resource_cached);
resource->mime_type = mime_type;
resource->is_primary_frame_resource = true;
resource->is_main_frame_resource =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,6 @@ void FrameData::ProcessResourceLoadInFrame(
const page_load_metrics::mojom::ResourceDataUpdatePtr& resource,
int process_id,
const page_load_metrics::ResourceTracker& resource_tracker) {
// TODO(968141): Update these metrics to include resources loaded by the
// memory cache.
if (resource->cache_type == page_load_metrics::mojom::CacheType::kMemory)
return;
bool is_same_origin = origin_.IsSameOriginWith(resource->origin);
bytes_ += resource->delta_bytes;
network_bytes_ += resource->delta_bytes;
Expand All @@ -111,8 +107,7 @@ void FrameData::ProcessResourceLoadInFrame(
num_resources_++;

// Report cached resource body bytes to overall frame bytes.
if (resource->is_complete &&
resource->cache_type != page_load_metrics::mojom::CacheType::kNotCached) {
if (resource->is_complete && resource->was_fetched_via_cache) {
bytes_ += resource->encoded_body_length;
if (is_same_origin)
same_origin_bytes_ += resource->encoded_body_length;
Expand All @@ -127,8 +122,7 @@ void FrameData::ProcessResourceLoadInFrame(
ad_network_bytes_ += resource->delta_bytes;
ad_bytes_ += resource->delta_bytes;
// Report cached resource body bytes to overall frame bytes.
if (resource->is_complete &&
resource->cache_type != page_load_metrics::mojom::CacheType::kNotCached)
if (resource->is_complete && resource->was_fetched_via_cache)
ad_bytes_ += resource->encoded_body_length;

ResourceMimeType mime_type = GetResourceMimeType(resource);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,44 +220,42 @@ const char kHistogramFirstNonScrollInputAfterFirstPaint[] =
const char kHistogramFirstScrollInputAfterFirstPaint[] =
"PageLoad.InputTiming.NavigationToFirstScroll.AfterPaint";

const char kHistogramPageLoadTotalBytes[] =
"PageLoad.Experimental.Bytes.Total2";
const char kHistogramPageLoadTotalBytes[] = "PageLoad.Experimental.Bytes.Total";
const char kHistogramPageLoadNetworkBytes[] =
"PageLoad.Experimental.Bytes.Network";
const char kHistogramPageLoadCacheBytes[] =
"PageLoad.Experimental.Bytes.Cache2";
const char kHistogramPageLoadCacheBytes[] = "PageLoad.Experimental.Bytes.Cache";
const char kHistogramPageLoadNetworkBytesIncludingHeaders[] =
"PageLoad.Experimental.Bytes.NetworkIncludingHeaders";
const char kHistogramPageLoadUnfinishedBytes[] =
"PageLoad.Experimental.Bytes.Unfinished";

const char kHistogramLoadTypeTotalBytesForwardBack[] =
"PageLoad.Experimental.Bytes.Total2.LoadType.ForwardBackNavigation";
"PageLoad.Experimental.Bytes.Total.LoadType.ForwardBackNavigation";
const char kHistogramLoadTypeNetworkBytesForwardBack[] =
"PageLoad.Experimental.Bytes.Network.LoadType.ForwardBackNavigation";
const char kHistogramLoadTypeCacheBytesForwardBack[] =
"PageLoad.Experimental.Bytes.Cache2.LoadType.ForwardBackNavigation";
"PageLoad.Experimental.Bytes.Cache.LoadType.ForwardBackNavigation";

const char kHistogramLoadTypeTotalBytesReload[] =
"PageLoad.Experimental.Bytes.Total2.LoadType.Reload";
"PageLoad.Experimental.Bytes.Total.LoadType.Reload";
const char kHistogramLoadTypeNetworkBytesReload[] =
"PageLoad.Experimental.Bytes.Network.LoadType.Reload";
const char kHistogramLoadTypeCacheBytesReload[] =
"PageLoad.Experimental.Bytes.Cache2.LoadType.Reload";
"PageLoad.Experimental.Bytes.Cache.LoadType.Reload";

const char kHistogramLoadTypeTotalBytesNewNavigation[] =
"PageLoad.Experimental.Bytes.Total2.LoadType.NewNavigation";
"PageLoad.Experimental.Bytes.Total.LoadType.NewNavigation";
const char kHistogramLoadTypeNetworkBytesNewNavigation[] =
"PageLoad.Experimental.Bytes.Network.LoadType.NewNavigation";
const char kHistogramLoadTypeCacheBytesNewNavigation[] =
"PageLoad.Experimental.Bytes.Cache2.LoadType.NewNavigation";
"PageLoad.Experimental.Bytes.Cache.LoadType.NewNavigation";

const char kHistogramTotalCompletedResources[] =
"PageLoad.Experimental.CompletedResources.Total2";
"PageLoad.Experimental.CompletedResources.Total";
const char kHistogramNetworkCompletedResources[] =
"PageLoad.Experimental.CompletedResources.Network";
const char kHistogramCacheCompletedResources[] =
"PageLoad.Experimental.CompletedResources.Cache2";
"PageLoad.Experimental.CompletedResources.Cache";

const char kHistogramInputToNavigation[] =
"PageLoad.Experimental.InputTiming.InputToNavigationStart";
Expand Down Expand Up @@ -771,8 +769,7 @@ void CorePageLoadMetricsObserver::OnResourceDataUseObserved(
resources) {
for (auto const& resource : resources) {
if (resource->is_complete) {
if (resource->cache_type ==
page_load_metrics::mojom::CacheType::kNotCached) {
if (!resource->was_fetched_via_cache) {
network_bytes_ += resource->encoded_body_length;
num_network_resources_++;
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -378,8 +378,7 @@ TEST_F(CorePageLoadMetricsObserverTest, Reload) {
int64_t cache_bytes = 0;
for (const auto& resource : resources) {
if (resource->is_complete) {
if (resource->cache_type ==
page_load_metrics::mojom::CacheType::kNotCached)
if (!resource->was_fetched_via_cache)
network_bytes += resource->encoded_body_length;
else
cache_bytes += resource->encoded_body_length;
Expand Down Expand Up @@ -458,8 +457,7 @@ TEST_F(CorePageLoadMetricsObserverTest, ForwardBack) {
int64_t cache_bytes = 0;
for (const auto& resource : resources) {
if (resource->is_complete) {
if (resource->cache_type ==
page_load_metrics::mojom::CacheType::kNotCached)
if (!resource->was_fetched_via_cache)
network_bytes += resource->encoded_body_length;
else
cache_bytes += resource->encoded_body_length;
Expand Down Expand Up @@ -532,8 +530,7 @@ TEST_F(CorePageLoadMetricsObserverTest, NewNavigation) {
int64_t cache_bytes = 0;
for (const auto& resource : resources) {
if (resource->is_complete) {
if (resource->cache_type ==
page_load_metrics::mojom::CacheType::kNotCached)
if (!resource->was_fetched_via_cache)
network_bytes += resource->encoded_body_length;
else
cache_bytes += resource->encoded_body_length;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -407,10 +407,7 @@ void DataReductionProxyMetricsObserverBase::OnResourceDataUseObserved(
resources) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
for (auto const& resource : resources) {
if (resource->cache_type == page_load_metrics::mojom::CacheType::kMemory)
continue;
if (resource->cache_type !=
page_load_metrics::mojom::CacheType::kNotCached) {
if (resource->was_fetched_via_cache) {
if (resource->is_complete) {
if (resource->is_secure_scheme) {
secure_cached_bytes_ += resource->encoded_body_length;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,7 @@ CreateDataReductionProxyResource(bool was_cached,
double compression_ratio) {
auto resource_data_update =
page_load_metrics::mojom::ResourceDataUpdate::New();
resource_data_update->cache_type =
was_cached ? page_load_metrics::mojom::CacheType::kHttp
: page_load_metrics::mojom::CacheType::kNotCached;
resource_data_update->was_fetched_via_cache = was_cached;
resource_data_update->delta_bytes = was_cached ? 0 : delta_bytes;
resource_data_update->encoded_body_length = delta_bytes;
resource_data_update->is_complete = is_complete;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -305,8 +305,7 @@ TEST_F(DataReductionProxyMetricsObserverTest, ByteInformationCompression) {
int64_t insecure_ocl_bytes = 0;
int64_t secure_ocl_bytes = 0;
for (const auto& request : resources) {
if (request->cache_type ==
page_load_metrics::mojom::CacheType::kNotCached) {
if (!request->was_fetched_via_cache) {
if (request->is_secure_scheme) {
secure_network_bytes += request->delta_bytes;
secure_ocl_bytes +=
Expand All @@ -323,9 +322,7 @@ TEST_F(DataReductionProxyMetricsObserverTest, ByteInformationCompression) {
}
if (request->proxy_used) {
drp_bytes += request->delta_bytes;
if (request->cache_type ==
page_load_metrics::mojom::CacheType::kNotCached &&
request->is_complete)
if (!request->was_fetched_via_cache && request->is_complete)
++drp_resources;
}
}
Expand Down Expand Up @@ -371,8 +368,7 @@ TEST_F(DataReductionProxyMetricsObserverTest, ByteInformationInflation) {
int64_t insecure_ocl_bytes = 0;
int64_t secure_ocl_bytes = 0;
for (const auto& request : resources) {
if (request->cache_type ==
page_load_metrics::mojom::CacheType::kNotCached) {
if (!request->was_fetched_via_cache) {
if (request->is_secure_scheme) {
secure_network_bytes += request->delta_bytes;
secure_ocl_bytes +=
Expand All @@ -392,9 +388,7 @@ TEST_F(DataReductionProxyMetricsObserverTest, ByteInformationInflation) {
secure_drp_bytes += request->delta_bytes;
else
drp_bytes += request->delta_bytes;
if (request->cache_type ==
page_load_metrics::mojom::CacheType::kNotCached &&
request->is_complete)
if (!request->was_fetched_via_cache && request->is_complete)
++drp_resources;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ void MediaPageLoadMetricsObserver::OnResourceDataUseObserved(
resources) {
for (auto const& resource : resources) {
if (resource->is_complete) {
if (resource->cache_type ==
page_load_metrics::mojom::CacheType::kNotCached)
if (!resource->was_fetched_via_cache)
network_bytes_ += resource->encoded_body_length;
else
cache_bytes_ += resource->encoded_body_length;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,7 @@ class MediaPageLoadMetricsObserverTest
SimulateResourceDataUseUpdate(resources);
for (const auto& resource : resources) {
if (resource->is_complete) {
if (resource->cache_type ==
page_load_metrics::mojom::CacheType::kNotCached)
if (!resource->was_fetched_via_cache)
network_bytes_ += resource->encoded_body_length;
else
cache_bytes_ += resource->encoded_body_length;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,9 +223,7 @@ void PreviewsPageLoadMetricsObserver::OnResourceDataUseObserved(
const std::vector<page_load_metrics::mojom::ResourceDataUpdatePtr>&
resources) {
for (auto const& resource : resources) {
if (resource->cache_type ==
page_load_metrics::mojom::CacheType::kNotCached &&
resource->is_complete) {
if (!resource->was_fetched_via_cache && resource->is_complete) {
num_network_resources_++;
}
total_network_bytes_ += resource->delta_bytes;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,46 +56,48 @@ void ResourceMetricsObserver::OnComplete(

void ResourceMetricsObserver::RecordResourceMimeHistograms(
const page_load_metrics::mojom::ResourceDataUpdatePtr& resource) {
bool was_cached =
resource->cache_type != page_load_metrics::mojom::CacheType::kNotCached;
int64_t data_length = was_cached ? resource->encoded_body_length
: resource->received_data_length;
int64_t data_length = resource->was_fetched_via_cache
? resource->encoded_body_length
: resource->received_data_length;
ResourceMimeType mime_type = FrameData::GetResourceMimeType(resource);
if (mime_type == ResourceMimeType::kImage) {
RESOURCE_BYTES_HISTOGRAM("Mime.Image", was_cached, data_length);
RESOURCE_BYTES_HISTOGRAM("Mime.Image", resource->was_fetched_via_cache,
data_length);
} else if (mime_type == ResourceMimeType::kJavascript) {
RESOURCE_BYTES_HISTOGRAM("Mime.JS", was_cached, data_length);
RESOURCE_BYTES_HISTOGRAM("Mime.JS", resource->was_fetched_via_cache,
data_length);
} else if (mime_type == ResourceMimeType::kVideo) {
RESOURCE_BYTES_HISTOGRAM("Mime.Video", was_cached, data_length);
RESOURCE_BYTES_HISTOGRAM("Mime.Video", resource->was_fetched_via_cache,
data_length);
} else if (mime_type == ResourceMimeType::kCss) {
RESOURCE_BYTES_HISTOGRAM("Mime.CSS", was_cached, data_length);
RESOURCE_BYTES_HISTOGRAM("Mime.CSS", resource->was_fetched_via_cache,
data_length);
} else if (mime_type == ResourceMimeType::kHtml) {
RESOURCE_BYTES_HISTOGRAM("Mime.HTML", was_cached, data_length);
RESOURCE_BYTES_HISTOGRAM("Mime.HTML", resource->was_fetched_via_cache,
data_length);
} else if (mime_type == ResourceMimeType::kOther) {
RESOURCE_BYTES_HISTOGRAM("Mime.Other", was_cached, data_length);
RESOURCE_BYTES_HISTOGRAM("Mime.Other", resource->was_fetched_via_cache,
data_length);
}
}

void ResourceMetricsObserver::RecordResourceHistograms(
const page_load_metrics::mojom::ResourceDataUpdatePtr& resource) {
// TODO(968141): Update these metrics to include resources loaded by the
// memory cache.
if (resource->cache_type == page_load_metrics::mojom::CacheType::kMemory)
return;
bool was_cached =
resource->cache_type != page_load_metrics::mojom::CacheType::kNotCached;
int64_t data_length = was_cached ? resource->encoded_body_length
: resource->received_data_length;
int64_t data_length = resource->was_fetched_via_cache
? resource->encoded_body_length
: resource->received_data_length;
if (resource->is_main_frame_resource && resource->reported_as_ad_resource) {
RESOURCE_BYTES_HISTOGRAM("Mainframe.AdResource", was_cached, data_length);
RESOURCE_BYTES_HISTOGRAM("Mainframe.AdResource",
resource->was_fetched_via_cache, data_length);
} else if (resource->is_main_frame_resource) {
RESOURCE_BYTES_HISTOGRAM("Mainframe.VanillaResource", was_cached,
data_length);
RESOURCE_BYTES_HISTOGRAM("Mainframe.VanillaResource",
resource->was_fetched_via_cache, data_length);
} else if (resource->reported_as_ad_resource) {
RESOURCE_BYTES_HISTOGRAM("Subframe.AdResource", was_cached, data_length);
RESOURCE_BYTES_HISTOGRAM("Subframe.AdResource",
resource->was_fetched_via_cache, data_length);
} else {
RESOURCE_BYTES_HISTOGRAM("Subframe.VanillaResource", was_cached,
data_length);
RESOURCE_BYTES_HISTOGRAM("Subframe.VanillaResource",
resource->was_fetched_via_cache, data_length);
}

// Only report sizes by mime type for ad resources.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,7 @@ void TabRestorePageLoadMetricsObserver::OnResourceDataUseObserved(
resources) {
for (auto const& resource : resources) {
if (resource->is_complete) {
if (resource->cache_type ==
page_load_metrics::mojom::CacheType::kNotCached)
if (!resource->was_fetched_via_cache)
network_bytes_ += resource->encoded_body_length;
else
cache_bytes_ += resource->encoded_body_length;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,7 @@ class TabRestorePageLoadMetricsObserverTest
SimulateResourceDataUseUpdate(resources);
for (const auto& resource : resources) {
if (resource->is_complete) {
if (resource->cache_type ==
page_load_metrics::mojom::CacheType::kNotCached)
if (!resource->was_fetched_via_cache)
network_bytes_ += resource->encoded_body_length;
else
cache_bytes_ += resource->encoded_body_length;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,7 @@ void UkmPageLoadMetricsObserver::OnResourceDataUseObserved(
return;
for (auto const& resource : resources) {
network_bytes_ += resource->delta_bytes;
if (resource->is_complete &&
resource->cache_type !=
page_load_metrics::mojom::CacheType::kNotCached) {
if (resource->is_complete && resource->was_fetched_via_cache) {
cache_bytes_ += resource->encoded_body_length;
}
}
Expand Down Expand Up @@ -390,7 +388,7 @@ void UkmPageLoadMetricsObserver::RecordTimingMetrics(
builder.SetCpuTime(total_foreground_cpu_time_.InMilliseconds());

// Use a bucket spacing factor of 1.3 for bytes.
builder.SetNet_CacheBytes2(ukm::GetExponentialBucketMin(cache_bytes_, 1.3));
builder.SetNet_CacheBytes(ukm::GetExponentialBucketMin(cache_bytes_, 1.3));
builder.SetNet_NetworkBytes2(
ukm::GetExponentialBucketMin(network_bytes_, 1.3));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1017,8 +1017,7 @@ TEST_F(UkmPageLoadMetricsObserverTest, PageSizeMetrics) {
int64_t network_bytes = 0;
int64_t cache_bytes = 0;
for (const auto& request : resources) {
if (request->cache_type ==
page_load_metrics::mojom::CacheType::kNotCached) {
if (!request->was_fetched_via_cache) {
network_bytes += request->delta_bytes;
} else {
cache_bytes += request->encoded_body_length;
Expand All @@ -1041,7 +1040,7 @@ TEST_F(UkmPageLoadMetricsObserverTest, PageSizeMetrics) {
GURL(kTestUrl1));
test_ukm_recorder().ExpectEntryMetric(kv.second.get(), "Net.NetworkBytes2",
bucketed_network_bytes);
test_ukm_recorder().ExpectEntryMetric(kv.second.get(), "Net.CacheBytes2",
test_ukm_recorder().ExpectEntryMetric(kv.second.get(), "Net.CacheBytes",
bucketed_cache_bytes);
}
}
Expand Down
Loading

0 comments on commit c87d16d

Please sign in to comment.