Prerender: Remove PerceivedPLT histograms

Prerender: Remove PerceivedPLT histograms

Reasons:
* We stopped looking at this class of metrics
* It saves 5320 bytes of the code size on Android (see below)
* Saves multi-GiB of data on UMA servers

Removing PerceivedPLT also made a bit of state unused, hence it was removed:
* PrerenderHistograms objects are now almost stateless
* PrerenderTabHelper now only manages origin_, url_ and swap_ticks_

To confirm the binary size change:
1. build chrome_apk with is_official_build = true
2. shell> find out/ReleaseOfficial/gen -name libchrome.so -exec size {} \;

BUG=698733, 698956, 603203
Review-Url: https://codereview.chromium.org/2738783002
Cr-Commit-Position: refs/heads/master@{#461105}
Committed: https://chromium.googlesource.com/chromium/src/+/7231325cc8642ceebcff903a590d3dccab5d7e65

[pasko, 2017-03-07 20:03:22]

The CQ bit was checked by pasko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 20:05:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 20:30:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 20:30:43]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pasko, 2017-03-08 11:09:08]

The CQ bit was checked by pasko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 11:09:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pasko, 2017-03-08 11:22:42]

The CQ bit was checked by pasko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 11:22:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 12:17:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 12:17:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pasko, 2017-03-08 20:24:48]

Description was changed from

==========
Prerender: Remove PerceivedPLT histograms

Reasons:
* We stopped looking at this class of metrics
* It saves 4704 bytes of the code size on Android as judged by the
  'size' utility on Linux.
* Saves multi-GiB of data on UMA servers

BUG=698733
==========

to

==========
Prerender: Remove PerceivedPLT histograms

Reasons:
* We stopped looking at this class of metrics
* It saves 4704 bytes of the code size on Android as judged by the
  'size' utility on Linux.
* Saves multi-GiB of data on UMA servers

BUG=698733
==========

[pasko, 2017-03-08 20:24:48]

pasko@chromium.org changed reviewers:
+ droger@chromium.org, mattcary@chromium.org

[mattcary, 2017-03-09 09:02:35]

lgtm

https://codereview.chromium.org/2738783002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (left):

https://codereview.chromium.org/2738783002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1124:
"Prerender.webcross_PrerenderNotSwappedInPLT", 1);
I guess we're losing verification here of not being swapped in. If we don't
deprecate prerender, we'll have to figure out another way to verify that.

https://codereview.chromium.org/2738783002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1514:
set_loader_path("/prerender/prerender_loader_with_replace_state.html");
I think you can remove this file from chrome/test/data?

[droger, 2017-03-09 09:55:25]

When I looked at this before, I came to the conclusion that the histograms were
used in tests to test the actual feature, and thus removing them would cost us
some test coverage.

Do you disagree with this? Or maybe we're fine with losing the test coverage?

[pasko, 2017-03-10 11:56:14]

On 2017/03/09 09:55:25, droger wrote:
> When I looked at this before, I came to the conclusion that the histograms
were
> used in tests to test the actual feature, and thus removing them would cost us
> some test coverage.
> 
> Do you disagree with this? Or maybe we're fine with losing the test coverage?

That's a good point. I do not want to loose coverage in general. I (naively)
made the mechanical change and did not pay much attention to specific points of
coverage we are loosing. I'll take a closer look. We should be able to find
cheaper alternatives to make sure coverage stays the same. Do you agree?

[mattcary, 2017-03-10 13:03:05]

On 2017/03/10 11:56:14, pasko wrote:
> On 2017/03/09 09:55:25, droger wrote:
> > When I looked at this before, I came to the conclusion that the histograms
> were
> > used in tests to test the actual feature, and thus removing them would cost
us
> > some test coverage.
> > 
> > Do you disagree with this? Or maybe we're fine with losing the test
coverage?
> 
> That's a good point. I do not want to loose coverage in general. I (naively)
> made the mechanical change and did not pay much attention to specific points
of
> coverage we are loosing. I'll take a closer look. We should be able to find
> cheaper alternatives to make sure coverage stays the same. Do you agree?

I looked very briefly when I refactored the tests, it seemed like alternatives
would be intrusive. Please prove me wrong!

[pasko, 2017-03-23 13:33:25]

https://codereview.chromium.org/2738783002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (left):

https://codereview.chromium.org/2738783002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1124:
"Prerender.webcross_PrerenderNotSwappedInPLT", 1);
On 2017/03/09 09:02:35, mattcary wrote:
> I guess we're losing verification here of not being swapped in. If we don't
> deprecate prerender, we'll have to figure out another way to verify that.

We actually perform a swap here in NavigateToDestURL(). The check is verifying
that the onload event fired before the swap happened. I think this also might be
flaky because I there might be no guaranteed ordering of the stoploading and fcp
IPCs.

I don't see why we need to verify that onload fired before swap in this case. It
was meaningful when we had histograms for it. I think in the current world
onload is similarly meaningless for prerender as domcontentloaded and other
loading progress events, hence I'm happy to remove it.

https://codereview.chromium.org/2738783002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1514:
set_loader_path("/prerender/prerender_loader_with_replace_state.html");
On 2017/03/09 09:02:35, mattcary wrote:
> I think you can remove this file from chrome/test/data?

Let me think more about this test case (gws prerender leads to a page with
client redirect). I noticed that we do not record PrefetchTTFCP. Do you know
why? I think it might be related to our skew in metrics.

[mattcary, 2017-03-23 13:49:27]

https://codereview.chromium.org/2738783002/diff/60001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (left):

https://codereview.chromium.org/2738783002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1124:
"Prerender.webcross_PrerenderNotSwappedInPLT", 1);
On 2017/03/23 13:33:24, pasko wrote:
> On 2017/03/09 09:02:35, mattcary wrote:
> > I guess we're losing verification here of not being swapped in. If we don't
> > deprecate prerender, we'll have to figure out another way to verify that.
> 
> We actually perform a swap here in NavigateToDestURL(). The check is verifying
> that the onload event fired before the swap happened. I think this also might
be
> flaky because I there might be no guaranteed ordering of the stoploading and
fcp
> IPCs.
> 
> I don't see why we need to verify that onload fired before swap in this case.
It
> was meaningful when we had histograms for it. I think in the current world
> onload is similarly meaningless for prerender as domcontentloaded and other
> loading progress events, hence I'm happy to remove it.

sgtm, thx for digging.

https://codereview.chromium.org/2738783002/diff/60001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1514:
set_loader_path("/prerender/prerender_loader_with_replace_state.html");
On 2017/03/23 13:33:25, pasko wrote:
> On 2017/03/09 09:02:35, mattcary wrote:
> > I think you can remove this file from chrome/test/data?
> 
> Let me think more about this test case (gws prerender leads to a page with
> client redirect). I noticed that we do not record PrefetchTTFCP. Do you know
> why? I think it might be related to our skew in metrics.

Hmmm. Redirects. We use extras.start_url in the PLMOs, for both prerender and
nostate. But if that isn't correct somehow, than for prerender we use the origin
from the tab helper and record under the Reference histogram (as prefetch_age is
zero), but for NoState/SimpleLoad we would put it under ORIGIN_NONE as well as
the Reference metric. Let me look at UMA and see if this theory is supported.

[mattcary, 2017-03-23 14:37:32]

> Hmmm. Redirects. We use extras.start_url in the PLMOs, for both prerender and
> nostate. But if that isn't correct somehow, than for prerender we use the
origin
> from the tab helper and record under the Reference histogram (as prefetch_age
is
> zero), but for NoState/SimpleLoad we would put it under ORIGIN_NONE as well as
> the Reference metric. Let me look at UMA and see if this theory is supported.

I'm not sure this theory holds up, but I have to keep digging. In my first try I
combined desktop & android metrics which I now think was a blunder.

[mattcary, 2017-03-24 12:26:54]

FYI, you're racing with droger@ on histogram-related changes. If he gets his
crrev/2769903005 in before you, you should sync and should be able to remove
WithinWindow() and related state (last_prerender_time).

[pasko, 2017-03-24 12:35:30]

On 2017/03/24 12:26:54, mattcary wrote:
> FYI, you're racing with droger@ on histogram-related changes. If he gets his
> crrev/2769903005 in before you, you should sync and should be able to remove
> WithinWindow() and related state (last_prerender_time).

He should get his change earlier (to make it cherry-pickable), I'd want to get
to the bottom of coverage from
PrerenderBrowserTest.PrerenderLocationReplaceGWSHistograms before proceeding
with this cleanup. Let's figure out why PrefetchTTFCP is not recorded. Another
option is to land the test in disabled mode and figure out details later.

[mattcary, 2017-03-24 13:10:35]

On 2017/03/24 12:35:30, pasko wrote:
> On 2017/03/24 12:26:54, mattcary wrote:
> > FYI, you're racing with droger@ on histogram-related changes. If he gets his
> > crrev/2769903005 in before you, you should sync and should be able to remove
> > WithinWindow() and related state (last_prerender_time).
> 
> He should get his change earlier (to make it cherry-pickable), I'd want to get
> to the bottom of coverage from
> PrerenderBrowserTest.PrerenderLocationReplaceGWSHistograms before proceeding
> with this cleanup. Let's figure out why PrefetchTTFCP is not recorded. Another
> option is to land the test in disabled mode and figure out details later.

I just looked into this issue using some cutting-edge printf debugging
technology I recently developed, and it seems that PrefetchTTFCP is not recorded
because... there is no FCP. Maybe related to the location change? I've tried
various tweaks to the test and the html it loads, and I can't get a FCP to fire
(at the page_load_tracker level). So I guess the test successfully evades our
FCP detection logic?

[pasko, 2017-03-27 12:33:50]

The CQ bit was checked by pasko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 12:34:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 12:36:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 12:36:27]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[pasko, 2017-03-27 14:26:11]

The CQ bit was checked by pasko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 14:26:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pasko, 2017-03-27 14:29:51]

On 2017/03/24 13:10:35, mattcary wrote:
> On 2017/03/24 12:35:30, pasko wrote:
> > On 2017/03/24 12:26:54, mattcary wrote:
> > > FYI, you're racing with droger@ on histogram-related changes. If he gets
his
> > > crrev/2769903005 in before you, you should sync and should be able to
remove
> > > WithinWindow() and related state (last_prerender_time).
> > 
> > He should get his change earlier (to make it cherry-pickable), I'd want to
get
> > to the bottom of coverage from
> > PrerenderBrowserTest.PrerenderLocationReplaceGWSHistograms before proceeding
> > with this cleanup. Let's figure out why PrefetchTTFCP is not recorded.
Another
> > option is to land the test in disabled mode and figure out details later.
> 
> I just looked into this issue using some cutting-edge printf debugging
> technology I recently developed, and it seems that PrefetchTTFCP is not
recorded
> because... there is no FCP. Maybe related to the location change? I've tried
> various tweaks to the test and the html it loads, and I can't get a FCP to
fire
> (at the page_load_tracker level). So I guess the test successfully evades our
> FCP detection logic?

I found that we are not getting the FCP because .. well .. we are not waiting
for it with the swap_observer. What a surprise. The test for PrefetchTTFCP
through a client redirect seems useful, so I restored it (even though it has
unrelated mechanics with the fancy delayed redirect on the image .. and is slow
because of that .. probably).

The histogram name to check in the test is modulo naming chosen in
https://codereview.chromium.org/2776713002/ (will take another look at it
shortly)

Removing these histograms also caused a snowball removing of unused state in
PrerenderHistograms (WithinWindow() and such). The only state left is
thread_checker_ which I think is still useful. Updated the description of the
class accordingly.

Remaining TBD - update the issue description to cover the wider cleanup and
maybe a few more bytes from the binary size are saved.

PTAL.

[mattcary, 2017-03-27 14:57:50]

See my comments in crbug/704911 as well for what to do about the reference
histogram naming.

https://codereview.chromium.org/2738783002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_tab_helper.cc (left):

https://codereview.chromium.org/2738783002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_tab_helper.cc:150:
DCHECK_EQ(NAVIGATION_TYPE_PRERENDERED, navigation_type_);
This is the only place navigation_type_ is used AFAICT. It doesn't seem useful
any more; I had to fiddle it a bit putting in the Prefetch histograms, and at
this point it doesn't add much over
PrerenderManager::IsWebContentsPrerendering().

[commit-bot: I haz the power, 2017-03-27 15:56:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 15:56:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pasko, 2017-03-27 16:37:20]

The CQ bit was checked by pasko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 16:37:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pasko, 2017-03-27 16:50:09]

The CQ bit was checked by pasko@chromium.org to run a CQ dry run

[pasko, 2017-03-27 16:50:40]

https://codereview.chromium.org/2738783002/diff/100001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_tab_helper.cc (left):

https://codereview.chromium.org/2738783002/diff/100001/chrome/browser/prerend...
chrome/browser/prerender/prerender_tab_helper.cc:150:
DCHECK_EQ(NAVIGATION_TYPE_PRERENDERED, navigation_type_);
On 2017/03/27 14:57:50, mattcary wrote:
> This is the only place navigation_type_ is used AFAICT. It doesn't seem useful
> any more; I had to fiddle it a bit putting in the Prefetch histograms, and at
> this point it doesn't add much over
> PrerenderManager::IsWebContentsPrerendering().

oh yes, thanks! I wanted to nuke that too. Forgot. Done.

[commit-bot: I haz the power, 2017-03-27 16:50:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pasko, 2017-03-27 17:22:20]

Description was changed from

==========
Prerender: Remove PerceivedPLT histograms

Reasons:
* We stopped looking at this class of metrics
* It saves 4704 bytes of the code size on Android as judged by the
  'size' utility on Linux.
* Saves multi-GiB of data on UMA servers

BUG=698733
==========

to

==========
Prerender: Remove PerceivedPLT histograms

Reasons:
* We stopped looking at this class of metrics
* It saves 5320 bytes of the code size on Android (see below)
* Saves multi-GiB of data on UMA servers

Removing PerceivedPLT also made a bit of state unused, hence it was removed:
* PrerenderHistograms objects are now almost stateless
* PrerenderTabHelper now only manages origin_, url_ and swap_ticks_

To confirm the binary size change:
1. build chrome_apk with is_official_build = true
2. shell> find out/ReleaseOfficial/gen -name libchrome.so -exec size {} \;

BUG=698733
==========

[commit-bot: I haz the power, 2017-03-27 17:25:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 17:25:11]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[pasko, 2017-03-28 14:47:07]

The CQ bit was checked by pasko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 14:47:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 15:49:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 15:50:00]

Dry run: This issue passed the CQ dry run.

[pasko, 2017-03-28 16:07:04]

tests are passing, PTAL

[pasko, 2017-03-29 17:01:55]

mattcary, droger: friendly ping :)

[mattcary, 2017-03-30 07:23:38]

On 2017/03/29 17:01:55, pasko wrote:
> mattcary, droger: friendly ping :)

still lgtm :)

[droger, 2017-03-30 08:57:53]

lgtm

[pasko, 2017-03-30 11:17:05]

The CQ bit was checked by pasko@chromium.org

[commit-bot: I haz the power, 2017-03-30 11:17:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 11:24:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 11:24:58]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pasko, 2017-03-30 11:53:50]

Description was changed from

==========
Prerender: Remove PerceivedPLT histograms

Reasons:
* We stopped looking at this class of metrics
* It saves 5320 bytes of the code size on Android (see below)
* Saves multi-GiB of data on UMA servers

Removing PerceivedPLT also made a bit of state unused, hence it was removed:
* PrerenderHistograms objects are now almost stateless
* PrerenderTabHelper now only manages origin_, url_ and swap_ticks_

To confirm the binary size change:
1. build chrome_apk with is_official_build = true
2. shell> find out/ReleaseOfficial/gen -name libchrome.so -exec size {} \;

BUG=698733
==========

to

==========
Prerender: Remove PerceivedPLT histograms

Reasons:
* We stopped looking at this class of metrics
* It saves 5320 bytes of the code size on Android (see below)
* Saves multi-GiB of data on UMA servers

Removing PerceivedPLT also made a bit of state unused, hence it was removed:
* PrerenderHistograms objects are now almost stateless
* PrerenderTabHelper now only manages origin_, url_ and swap_ticks_

To confirm the binary size change:
1. build chrome_apk with is_official_build = true
2. shell> find out/ReleaseOfficial/gen -name libchrome.so -exec size {} \;

BUG=698733,698956,603203
==========

[pasko, 2017-03-30 11:55:12]

pasko@chromium.org changed reviewers:
+ isherman@chromium.org

[pasko, 2017-03-30 11:55:41]

isherman: please take a look at changes in histograms.xml

[Ilya Sherman, 2017-03-30 22:02:13]

histograms.xml lgtm -- thanks for the cleanup!

[pasko, 2017-03-31 12:39:26]

The CQ bit was checked by pasko@chromium.org

[commit-bot: I haz the power, 2017-03-31 12:39:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 13:49:56]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490963966729330,
"parent_rev": "1c0d3dd9ac676ade69a0418a089b79f1fa50205c", "commit_rev":
"7231325cc8642ceebcff903a590d3dccab5d7e65"}

[commit-bot: I haz the power, 2017-03-31 13:50:43]

Description was changed from

==========
Prerender: Remove PerceivedPLT histograms

Reasons:
* We stopped looking at this class of metrics
* It saves 5320 bytes of the code size on Android (see below)
* Saves multi-GiB of data on UMA servers

Removing PerceivedPLT also made a bit of state unused, hence it was removed:
* PrerenderHistograms objects are now almost stateless
* PrerenderTabHelper now only manages origin_, url_ and swap_ticks_

To confirm the binary size change:
1. build chrome_apk with is_official_build = true
2. shell> find out/ReleaseOfficial/gen -name libchrome.so -exec size {} \;

BUG=698733,698956,603203
==========

to

==========
Prerender: Remove PerceivedPLT histograms

Reasons:
* We stopped looking at this class of metrics
* It saves 5320 bytes of the code size on Android (see below)
* Saves multi-GiB of data on UMA servers

Removing PerceivedPLT also made a bit of state unused, hence it was removed:
* PrerenderHistograms objects are now almost stateless
* PrerenderTabHelper now only manages origin_, url_ and swap_ticks_

To confirm the binary size change:
1. build chrome_apk with is_official_build = true
2. shell> find out/ReleaseOfficial/gen -name libchrome.so -exec size {} \;

BUG=698733,698956,603203

Review-Url: https://codereview.chromium.org/2738783002
Cr-Commit-Position: refs/heads/master@{#461105}
Committed:
https://chromium.googlesource.com/chromium/src/+/7231325cc8642ceebcff903a590d...
==========

[commit-bot: I haz the power, 2017-03-31 13:50:44]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/7231325cc8642ceebcff903a590d...