[page_load_metrics] Log cache warmth ratios at parse stop (issue 2218583002 by csharrison@chromium.org)

[cshar...@chromium.org]

Reviewers: kinuko
CL: https://codereview.chromium.org/2218583002/

Message:
kinuko@, do you have time to take a look at this?

My thinking is that we first get a sense of cache ratios for page loads in
general (I chose parse end because lots of our efforts are targeting parsing).

Then, in a followup change, we can start separating out PLT metrics based on the
warmth ratio. I'm hopeful that this will allow Caminito / NeverIdle to zero in
on the loads they are hoping to affect.

Description:
[page_load_metrics] Log cache warmth ratios at parse stop

This patch instruments ChromeResourceDispatcherHostDelegate to forward
request information upon request completion to page_load_metrics. This
data is then associated with the current committed navigation, and used
to generate histograms of % cache warmth in terms of total requests and
total bytes loaded.

Note: This CL also coalesces short events posted to the UI thread to avoid
spamming the task queue on request complete.

BUG=634120

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+132, -25 lines):
M chrome/browser/page_load_metrics/metrics_web_contents_observer.h
M chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
M chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
M chrome/browser/page_load_metrics/page_load_metrics_observer.h
M chrome/browser/page_load_metrics/page_load_metrics_observer.cc
M chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc

[kin...@chromium.org]

The idea sounds good but I'm a bit concerned about the approach, I'm not sure if
the amount of bytes would show meaningful correlation with loading latency?
Say, suppose we have 100 requests and a few requests are served from cache and
the content size was about 1K in total, while we needed to load 90+ requests
from network but all were very small, say 10 bytes for each, the cache warmth
measured by # of bytes really represent what we want to know?

https://codereview.chromium.org/2218583002/

[cshar...@chromium.org]

Yes I had the same concern, which is why I had a ratio of requests served from
cache as well. I think that's the more interesting statistic here, but I
included the byte count as reference. If it turns out to be useless we can
remove that one.

https://codereview.chromium.org/2218583002/

[Bryan McQuade]

We also track time the parser is blocked on script load. Could potentially use ratio of script load block time up to parse complete as a signal as well.

--
You received this message because you are subscribed to the Google Groups "loading-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email to loading-revie...@chromium.org.
To post to this group, send email to loading...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/loading-reviews/94eb2c0bb3aeeb2b9b0539546d72%40google.com.

[cshar...@chromium.org]

True, but I still think a holistic approach that also counts images/css/etc is
also important. I'd be curious to see the correlation between this metric and
the one you mention, Bryan.

https://codereview.chromium.org/2218583002/

[bmcq...@chromium.org]

On 2016/08/05 at 15:31:11, csharrison wrote:
> True, but I still think a holistic approach that also counts images/css/etc is
also important. I'd be curious to see the correlation between this metric and
the one you mention, Bryan.

yes, we should really understand css network blocking time in addition to script
blocking time. i'm less sure how important image time from network vs cache will
be for the progressive web metrics.
we can consider adding the '% of time parser blocked on network' dimension in
the future - not needed for this change.

https://codereview.chromium.org/2218583002/

[cshar...@chromium.org]

On 2016/08/05 15:34:06, Bryan McQuade wrote:
> On 2016/08/05 at 15:31:11, csharrison wrote:
> > True, but I still think a holistic approach that also counts images/css/etc
is
> also important. I'd be curious to see the correlation between this metric and
> the one you mention, Bryan.
>
> yes, we should really understand css network blocking time in addition to
script
> blocking time. i'm less sure how important image time from network vs cache
will
> be for the progressive web metrics.

I think I disagree wrt first meaningful paint at least. I imagine an image heavy
site will definitely show metrics change if the images are cached or not, which
will allow CPU bound loading efforts to show through the noise generated by
network bound loads.

> we can consider adding the '% of time parser blocked on network' dimension in
> the future - not needed for this change.

SGTM

https://codereview.chromium.org/2218583002/

[Bryan McQuade]

I don't recall how/whether FMP takes image loads into account... I need to read more about that.

[cshar...@chromium.org]

At least many image loads will cause re-layouts right? That will affect FMP for
sure.

https://codereview.chromium.org/2218583002/

[cshar...@chromium.org]

kinuko@ would you like me to remove the byte ratio metric? Are you okay with the
patch approach otherwise?

https://codereview.chromium.org/2218583002/

[bmcq...@chromium.org]

https://codereview.chromium.org/2218583002/diff/20001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2218583002/diff/20001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#newcode763
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:763:
int64_t bytes = was_cached ? raw_body_bytes : total_received_bytes;
is raw_body_bytes uncompressed but total_received_bytes compressed?
that'd skew byte counts towards cached responses.

https://codereview.chromium.org/2218583002/diff/20001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#newcode764
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:764:
committed_load_->OnLoadedSubresource(bytes, was_cached);
are we sure that all resources loaded are for the currently committed
load? can we verify that somehow?

https://codereview.chromium.org/2218583002/diff/20001/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2218583002/diff/20001/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc#newcode162
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:162:
"PageLoad.Cache.RequestPercent.ParseStop";
can we call these 'PageLoad.Experimental.Cache' for now? once we're
happy with them we can promote them out of experimental.

https://codereview.chromium.org/2218583002/diff/20001/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc#newcode369
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:369:
UMA_HISTOGRAM_PERCENTAGE(internal::kHistogramCacheRequestPercentParseStop,
one tricky thing with percentages is if you get something like 1/2
that's 50% which ends up being the same as 25/50 - I'd claim that a page
with 25 resources served from cache has very different performance
characteristics from a page that has 1 served out of cache. To get
better insight here, can we also add some counters? It'd be interesting
for instance to count number of pages that have a very small number of
total resources - we might want to discard those pages from the
percentage histogram.

https://codereview.chromium.org/2218583002/

[cshar...@chromium.org]

Thanks Bryan! Don't feel obliged to review this I know you're OOO.

https://codereview.chromium.org/2218583002/diff/20001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2218583002/diff/20001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#newcode763
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:763:
int64_t bytes = was_cached ? raw_body_bytes : total_received_bytes;

On 2016/08/08 18:53:28, Bryan McQuade wrote:
> is raw_body_bytes uncompressed but total_received_bytes compressed?
that'd skew
> byte counts towards cached responses.

Great point. I chatted with mmenke@ about this and it seems like if we
want decompressed bytes we have to instrument read calls manually here.
Because that's a decent amount of code for little gain, I'll just punt
on the "bytes" metric for now and go based on # of requests.

https://codereview.chromium.org/2218583002/diff/20001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#newcode764
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:764:
committed_load_->OnLoadedSubresource(bytes, was_cached);

On 2016/08/08 18:53:28, Bryan McQuade wrote:
> are we sure that all resources loaded are for the currently committed
load? can
> we verify that somehow?

I think this is reasonably accurate. If there are subresource requests
going on I think it makes sense to attribute them with the current page
(i.e. current committed load). Barring threading race conditions, I
think we're okay here.

What kind of verification were you thinking of?

The alternative approach here would be to do this logic in blink. But I
wanted to fully account for OOPIF, and subframe loads contribute lots to
page loads.

https://codereview.chromium.org/2218583002/diff/20001/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2218583002/diff/20001/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc#newcode162
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:162:
"PageLoad.Cache.RequestPercent.ParseStop";

On 2016/08/08 18:53:28, Bryan McQuade wrote:
> can we call these 'PageLoad.Experimental.Cache' for now? once we're
happy with
> them we can promote them out of experimental.

Done.

https://codereview.chromium.org/2218583002/

[kin...@chromium.org]

So we're dropping bytes data for now? Could you update the issue description as
well?


https://codereview.chromium.org/2218583002/diff/40001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2218583002/diff/40001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#newcode749
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:749:
// For simplicity, only count subresources.
nit: this comment doesn't really say anything, would be nice to explain
why we only care about subresources

https://codereview.chromium.org/2218583002/diff/40001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h
File chrome/browser/page_load_metrics/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2218583002/diff/40001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h#newcode273
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:273:
int network_requests_;
nit: maybe add num_ or _count

https://codereview.chromium.org/2218583002/diff/40001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h#newcode330
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:330: //
For definitions of total_received_bytes and raw_body_bytes, see the
nit: stale comments

https://codereview.chromium.org/2218583002/

[cshar...@chromium.org]

Thanks for the review!

https://codereview.chromium.org/2218583002/diff/40001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2218583002/diff/40001/chrome/browser/page_load_metrics/metrics_web_contents_observer.cc#newcode749
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:749:
// For simplicity, only count subresources.

On 2016/08/09 12:40:37, kinuko wrote:
> nit: this comment doesn't really say anything, would be nice to
explain why we
> only care about subresources

I added a comment. Essentially we do care about main resource loads but
they will have to be treated differently / more complicated within this
pipeline because data comes in from main resource loads before we have a
committed load here.

https://codereview.chromium.org/2218583002/diff/40001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h
File chrome/browser/page_load_metrics/metrics_web_contents_observer.h
(right):

https://codereview.chromium.org/2218583002/diff/40001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h#newcode273
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:273:
int network_requests_;

On 2016/08/09 12:40:37, kinuko wrote:
> nit: maybe add num_ or _count

Done.

https://codereview.chromium.org/2218583002/diff/40001/chrome/browser/page_load_metrics/metrics_web_contents_observer.h#newcode330
chrome/browser/page_load_metrics/metrics_web_contents_observer.h:330: //
For definitions of total_received_bytes and raw_body_bytes, see the

On 2016/08/09 12:40:37, kinuko wrote:
> nit: stale comments

Done.

https://codereview.chromium.org/2218583002/

[kin...@chromium.org]

The code change lgtm, for the validity of the metrics let's keep watching what
it'd show

https://codereview.chromium.org/2218583002/

[cshar...@chromium.org]

Thanks, agreed. I did some manual testing with devtools and things lined up
pretty well, but let's see what we get from the wild.

+thestig for chrome_resource_dispatcher_host_delegate
+holte for histograms.xml

PTAL thank you.

https://codereview.chromium.org/2218583002/

[cshar...@chromium.org]

Actually +holte for histograms.xml

https://codereview.chromium.org/2218583002/

[the...@chromium.org]

somewhat-rubberstampish lgtm


https://codereview.chromium.org/2218583002/diff/60001/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
File
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/2218583002/diff/60001/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc#newcode283
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:283:
// This function is called in RequestComplete to log metrics about main
frame
This comment is slightly out of date?

https://codereview.chromium.org/2218583002/diff/60001/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc#newcode289
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:289:
if (!web_contents)
Seems this should never eval to false now.

https://codereview.chromium.org/2218583002/

[cshar...@chromium.org]

Thank you :)

https://codereview.chromium.org/2218583002/diff/60001/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
File
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/2218583002/diff/60001/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc#newcode283
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:283:
// This function is called in RequestComplete to log metrics about main
frame

On 2016/08/09 19:14:17, Lei Zhang wrote:
> This comment is slightly out of date?

Updated.

https://codereview.chromium.org/2218583002/diff/60001/chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc#newcode289
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:289:
if (!web_contents)

On 2016/08/09 19:14:17, Lei Zhang wrote:
> Seems this should never eval to false now.

Removed.

https://codereview.chromium.org/2218583002/

[ho...@chromium.org]

histograms lgtm

https://codereview.chromium.org/2218583002/

[cshar...@chromium.org]

Thanks!

https://codereview.chromium.org/2218583002/