Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2776713002:
Prerender: clarify prerender origin for purposes of FCP histograms. (Closed)
DescriptionPrerender: Clarify prerender origin for purposes of FCP histograms.
This CL changes the origin used for prerender PrefetchTTFCP
histograms. Previously, the orgin from the prerender_tab_helper was used. This
meant that a page that is not found in the prefetches list, and assigned a
Reference cache type, can have some origin other than NONE for prerender. With
NoStatePrefetch or disabled prerender experiments, this is impossible and all
Reference histograms have origin NONE.
We believe the main case for a discrepency is when there is a redirect in the
prererendered page. The URL seen at FCP time is different than what was recorded
in our list of prefetched pages.
BUG=704911
Patch Set 1 #
Total comments: 3
Messages
Total messages: 12 (3 generated)
Description was changed from ========== Prerender: clarify prerender origin for purposes of FCP histograms. This CL changes the origin used for prerender PrefetchTTFCP histograms. Previously, the orgin from the prerender_tab_helper was used. This meant that a page that is not found in the prefetches list, and assigned a Reference cache type, can have some origin other than NONE for prerender. With NoStatePrefetch or disabled prerender experiments, this is impossible and all Reference histograms have origin NONE. We believe the main case for a discrepency is when there is a redirect in the prererendered page. The URL seen at FCP time is different than what was recorded in our list of prefetched pages. BUG=704911 ========== to ========== Prerender: Clarify prerender origin for purposes of FCP histograms. This CL changes the origin used for prerender PrefetchTTFCP histograms. Previously, the orgin from the prerender_tab_helper was used. This meant that a page that is not found in the prefetches list, and assigned a Reference cache type, can have some origin other than NONE for prerender. With NoStatePrefetch or disabled prerender experiments, this is impossible and all Reference histograms have origin NONE. We believe the main case for a discrepency is when there is a redirect in the prererendered page. The URL seen at FCP time is different than what was recorded in our list of prefetched pages. BUG=704911 ==========
mattcary@chromium.org changed reviewers: + droger@chromium.org, pasko@chromium.org
As discussed offline, this makes prerender consistent with prefetching, but does not actually fix the redirect issue. So this may not be the right solution, comments welcome.
On 2017/03/24 14:26:45, mattcary wrote: > As discussed offline, this makes prerender consistent with prefetching, but does > not actually fix the redirect issue. So this may not be the right solution, > comments welcome. This is all very puzzling :) It took me hours to understand that none_PrefetchTTFCP.Reference for Control group is currently recorded via PrerenderManager::RecordNoStateFirstContentfulPaint(), and I think I came to this conclusion several times, forgetting the previous along the way. Is it true or I am reading it wrong? Related to redirects: I think if a page reached FCP without a client redirect, then we would correctly match it with prefetches_. I think so because in both groups we are checking prefetches_ against extra_info.start_url, which seems to be what was stored before any redirect was made: https://codesearch.chromium.org/chromium/src/chrome/browser/page_load_metrics... https://codereview.chromium.org/2776713002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2776713002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_manager.cc:612: if (!swap_ticks.is_null() && !first_contentful_paint.is_null()) { this is only recorded when swap happened (!swap_ticks.is_null()), so it would not be able to record "Reference", right? then why changing an origin here matters for "Reference"?
> This is all very puzzling :)
>
> It took me hours to understand that none_PrefetchTTFCP.Reference for Control
> group is currently recorded via
> PrerenderManager::RecordNoStateFirstContentfulPaint(), and I think I came to
> this conclusion several times, forgetting the previous along the way.
>
> Is it true or I am reading it wrong?
>
That's true. There are three main FCPs that get recorded: one from CorePLMO, one
from NoStatePLMO and one from Prerender PLMO. The latter is only enabled if the
WebContents is prerendering, and in that case the first to PLMOs aren't
attached. The NoStatePLMO should be read as "FCP metrics related to the NoState
experiment" and *not* "FCP metrics only recorded if a NoState prefetch is being
done". It's done this way so that we can be as consistent as possible for
SimpleLoad recording prefetch age, etc. This is particularly because from the
point of view of FCP, NoState is no different than a SimpleLoad, there just
happens to be resources in the cache. That is, the renderer that does the
NoState prefetch isn't involved in the FCP.
The same is true in Control when a prerender wasn't actually involved (ie,
none_PrefetchTTFCP.Reference). The WebContents in question is a normal one and
not a prerenderer, even though a prerender "could have happened" (scare quotes
to elide more precise reasoning about counterfactuals). The essential problem is
that there isn't just a "FCP in the control" and "FCP in the treatment"; all
branches in the experiment have several different kinds of paints that we wanted
to capture in the {6*3*2*2} different FCP metrics.
> Related to redirects:
>
> I think if a page reached FCP without a client redirect, then we would
correctly
> match it with prefetches_. I think so because in both groups we are checking
> prefetches_ against extra_info.start_url, which seems to be what was stored
> before any redirect was made:
>
>
https://codesearch.chromium.org/chromium/src/chrome/browser/page_load_metrics...
>
Yes, that's correct. But the client-side redirects (by which I mean javascript
redirects) would not be captured in start_url. If there is a client-side meta
redirect, that also won't affect start_url I think? And so would the be
consistent for prerender, which would render the refreshed page, and when FCP
fired the start_url would be for the refreshed page, not the original one, and
so would get recorded under none_*.Reference? This would mean in our metrics we
miss a case where prerender actually does better than nostate, but we bury it in
the none_*.Reference noise.
So where has that left us?
(*) we want the origin to be recorded consistently, which this CL does, and
(*) in the case of redirects we are consistent between nostate, prerender, and
simpleload, even if we are missing a possibly minor case?
>
https://codereview.chromium.org/2776713002/diff/1/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_manager.cc (right):
>
>
https://codereview.chromium.org/2776713002/diff/1/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_manager.cc:612: if (!swap_ticks.is_null()
&&
> !first_contentful_paint.is_null()) {
> this is only recorded when swap happened (!swap_ticks.is_null()), so it would
> not be able to record "Reference", right?
>
> then why changing an origin here matters for "Reference"?
https://codereview.chromium.org/2776713002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2776713002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_manager.cc:612: if (!swap_ticks.is_null() && !first_contentful_paint.is_null()) { On 2017/03/24 16:10:30, pasko wrote: > this is only recorded when swap happened (!swap_ticks.is_null()), so it would > not be able to record "Reference", right? > > then why changing an origin here matters for "Reference"? Or, it could perhaps be that my logic on the client-side redirect is wrong, and in the prerender case we retain the original URL of the webcontents?
On 2017/03/27 07:52:05, mattcary wrote: > https://codereview.chromium.org/2776713002/diff/1/chrome/browser/prerender/pr... > File chrome/browser/prerender/prerender_manager.cc (right): > > https://codereview.chromium.org/2776713002/diff/1/chrome/browser/prerender/pr... > chrome/browser/prerender/prerender_manager.cc:612: if (!swap_ticks.is_null() && > !first_contentful_paint.is_null()) { > On 2017/03/24 16:10:30, pasko wrote: > > this is only recorded when swap happened (!swap_ticks.is_null()), so it would > > not be able to record "Reference", right? > > > > then why changing an origin here matters for "Reference"? > > Or, it could perhaps be that my logic on the client-side redirect is wrong, and > in the prerender case we retain the original URL of the webcontents? To clarify, we are getting non-none Reference counts from prerender, isn't this the only place it could happen? We get a Reference classification when prefetch_age is null, which happens when the url isn't found in the prefetch list (in GetPrefetchInformation()).
https://codereview.chromium.org/2776713002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_manager.cc (right): https://codereview.chromium.org/2776713002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_manager.cc:612: if (!swap_ticks.is_null() && !first_contentful_paint.is_null()) { On 2017/03/27 07:52:05, mattcary wrote: > On 2017/03/24 16:10:30, pasko wrote: > > this is only recorded when swap happened (!swap_ticks.is_null()), so it would > > not be able to record "Reference", right? > > > > then why changing an origin here matters for "Reference"? > > Or, it could perhaps be that my logic on the client-side redirect is wrong, and > in the prerender case we retain the original URL of the webcontents? Sorry, I misread. We are recording "Reference" with Prerender in presence of a client redirect (because the new PageLoadExtraInfo.start_url does not match anything in prefetches_).
Pursuant to discussion on the bug, I'll put this CL on ice. After we see the stats post-WASH removal, we'll probably either keep status quo, or get nostate to record this case accurately. I won't delete this CL yet, but will remove you as a reviewer so it doesn't clutter up your box.
mattcary@chromium.org changed reviewers: - droger@chromium.org, pasko@chromium.org
On 2017/03/28 07:22:59, mattcary wrote: > I won't delete this CL yet, but will remove you as a reviewer so it doesn't > clutter up your box. You can use the "close CL" checkbox which may be what you are looking for. It will no longer show up in our active CL list but we'll still receive emails if someone adds a comment. I don't think you can delete a CL anyway.
Message was sent while issue was closed.
On 2017/03/28 08:50:36, droger wrote: > On 2017/03/28 07:22:59, mattcary wrote: > > I won't delete this CL yet, but will remove you as a reviewer so it doesn't > > clutter up your box. > > You can use the "close CL" checkbox which may be what you are looking for. It > will no longer show up in our active CL list but we'll still receive emails if > someone adds a comment. I don't think you can delete a CL anyway. agreed: closing is the most convenient. It is possible to reopen with "Edit issue" link. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
