Reports Prerender and Profile Total Bytes

Reports UMA data for bytes fetched over the network for used and
unused prerenders.  It also reports on the total number of bytes
fetched for that profile since the last prerender report.

This gives us insight into the balance of used vs wasted prerenders.
Understanding the ratio of wasted prerender bytes to total bytes gives
us a better understanding of whether we need an option to disable
prerender on mobile networks.

BUG=334602
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251316

[tburkard, 2014-02-12 19:54:55]

looks pretty good so far, just a few nits.

also, asvitkine should review histograms.xml

https://codereview.chromium.org/160513002/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/160513002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_histograms.cc:417:
UMA_HISTOGRAM_COUNTS("Prerender.NetworkBytes.Used.KB",
nit: shouldnt { } be used if it's more than one line?

https://codereview.chromium.org/160513002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_histograms.cc:423: int64 recent_profile_bytes
= profile_bytes - last_profile_network_bytes_;
I would keep PrerenderHistograms dead simple, and literally just emit the
histogram.

I'd put this logic into prerender_manager, similar to the way you put it into
prerender_contents in the other case.

https://codereview.chromium.org/160513002/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_histograms.h (right):

https://codereview.chromium.org/160513002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_histograms.h:115: void RecordBytes(bool used,
int64 prerender_bytes, int64 profile_bytes);
nit: Perhaps a more descriptive name, like RecordNetworkBytes

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

https://codereview.chromium.org/160513002/diff/1/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:133: 
Couldnt you this be done more easily by using FindPrerenderContents(..) above,
and then calling its 
->prerender_manager()?

https://codereview.chromium.org/160513002/diff/1/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:167:
GetPrerenderManager(render_process_id, render_view_id);
You could remove GetPrerenderManager() entirely, and just replace
GetPrerenderManager with prerender_contents->prerender_manager() here

https://codereview.chromium.org/160513002/diff/1/content/browser/loader/resou...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/160513002/diff/1/content/browser/loader/resou...
content/browser/loader/resource_dispatcher_host_impl.cc:790: loader->request(),
info->GetChildID(), info->GetRouteID());
nit: isnt child/route id also in the request, in extra info? If yes and you
could use that, maybe easier to just pass the request rather than the additional
info that is (maybe) also inside the request.

[jkarlin, 2014-02-13 13:24:49]

https://codereview.chromium.org/160513002/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/160513002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_histograms.cc:417:
UMA_HISTOGRAM_COUNTS("Prerender.NetworkBytes.Used.KB",
On 2014/02/12 19:54:56, tburkard wrote:
> nit: shouldnt { } be used if it's more than one line?

Done.

https://codereview.chromium.org/160513002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_histograms.cc:423: int64 recent_profile_bytes
= profile_bytes - last_profile_network_bytes_;
On 2014/02/12 19:54:56, tburkard wrote:
> I would keep PrerenderHistograms dead simple, and literally just emit the
> histogram.
> 
> I'd put this logic into prerender_manager, similar to the way you put it into
> prerender_contents in the other case.

Done.

https://codereview.chromium.org/160513002/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_histograms.h (right):

https://codereview.chromium.org/160513002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_histograms.h:115: void RecordBytes(bool used,
int64 prerender_bytes, int64 profile_bytes);
On 2014/02/12 19:54:56, tburkard wrote:
> nit: Perhaps a more descriptive name, like RecordNetworkBytes

Done.

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

https://codereview.chromium.org/160513002/diff/1/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:133: 
On 2014/02/12 19:54:56, tburkard wrote:
> Couldnt you this be done more easily by using FindPrerenderContents(..) above,
> and then calling its 
> ->prerender_manager()?

Yes! Done.

https://codereview.chromium.org/160513002/diff/1/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:167:
GetPrerenderManager(render_process_id, render_view_id);
On 2014/02/12 19:54:56, tburkard wrote:
> You could remove GetPrerenderManager() entirely, and just replace
> GetPrerenderManager with prerender_contents->prerender_manager() here

Done.

https://codereview.chromium.org/160513002/diff/1/content/browser/loader/resou...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/160513002/diff/1/content/browser/loader/resou...
content/browser/loader/resource_dispatcher_host_impl.cc:790: loader->request(),
info->GetChildID(), info->GetRouteID());
On 2014/02/12 19:54:56, tburkard wrote:
> nit: isnt child/route id also in the request, in extra info? If yes and you
> could use that, maybe easier to just pass the request rather than the
additional
> info that is (maybe) also inside the request.

Done.

[tburkard, 2014-02-13 13:39:34]

lgtm

please wait for alexei's feedback re: histograms.xml though

https://codereview.chromium.org/160513002/diff/160001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/160513002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1775: int64 recent_profile_bytes =
Maybe add a DCHECK that recent_profile_bytes is >= 0?

https://codereview.chromium.org/160513002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1783: profile_network_bytes_ +=
bytes;
Add a DCHECK that bytes is >= 0.

[jkarlin, 2014-02-13 13:48:09]

https://codereview.chromium.org/160513002/diff/160001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/160513002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1775: int64 recent_profile_bytes =
On 2014/02/13 13:39:35, tburkard wrote:
> Maybe add a DCHECK that recent_profile_bytes is >= 0?

Done.

https://codereview.chromium.org/160513002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.cc:1783: profile_network_bytes_ +=
bytes;
On 2014/02/13 13:39:35, tburkard wrote:
> Add a DCHECK that bytes is >= 0.

Done.

[jkarlin, 2014-02-13 13:49:17]

darin@chromium.org: Please review changes in contents/

[jkarlin, 2014-02-13 13:51:05]

On 2014/02/13 13:49:17, jkarlin wrote:
> darin@chromium.org: Please review changes in contents/

darin: Sorry, that wasn't quite right. Please review everything save for
prerender/ and histograms/

[Alexei Svitkine (slow), 2014-02-13 15:50:49]

https://codereview.chromium.org/160513002/diff/330001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/160513002/diff/330001/chrome/browser/prerende...
chrome/browser/prerender/prerender_histograms.cc:417: prerender_bytes >> 10);
Wouldn't you be losing the remainder bytes each time you log this?

Perhaps it would be better to log the raw byte counts instead, rather than the
KB?

Alternatively, maybe you can keep a running total of the remainder that you add
to the value each time its logged (and update the new remainder)?

[jkarlin, 2014-02-13 16:04:14]

https://codereview.chromium.org/160513002/diff/330001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/160513002/diff/330001/chrome/browser/prerende...
chrome/browser/prerender/prerender_histograms.cc:417: prerender_bytes >> 10);
On 2014/02/13 15:50:50, Alexei Svitkine wrote:
> Wouldn't you be losing the remainder bytes each time you log this?
> 
> Perhaps it would be better to log the raw byte counts instead, rather than the
> KB?
> 
> Alternatively, maybe you can keep a running total of the remainder that you
add
> to the value each time its logged (and update the new remainder)?

Done.

[Alexei Svitkine (slow), 2014-02-13 16:28:43]

https://codereview.chromium.org/160513002/diff/460001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/160513002/diff/460001/chrome/browser/prerende...
chrome/browser/prerender/prerender_histograms.cc:416:
UMA_HISTOGRAM_COUNTS("Prerender.NetworkBytes.Used", prerender_bytes);
The default UMA_HISTOGRAM_COUNTS() macro defines a max of 1000000, which is
probably too low when you're logging bytes (i.e. I would expect you might get
enough records above that, that it's worth using a higher max).

Probably, 100M is a good max to use (by reasoning of 10M still possibly being
too low for some pages, so next magnitude).

Change these to:

const int kHistogramMin = 1;
const int kHistogramMax = 100000000;  // 100M.
const int kBucketCount = 50;

UMA_HISTOGRAM_CUSTOM_COUNTS("Prerender.NetworkBytes.Used", prerender_bytes,
                            kHistogramMin, kHistogramMax, kBucketCount)

[jkarlin, 2014-02-13 17:27:51]

https://codereview.chromium.org/160513002/diff/460001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/160513002/diff/460001/chrome/browser/prerende...
chrome/browser/prerender/prerender_histograms.cc:416:
UMA_HISTOGRAM_COUNTS("Prerender.NetworkBytes.Used", prerender_bytes);
Done.  Thanks!

On 2014/02/13 16:28:44, Alexei Svitkine wrote:
> The default UMA_HISTOGRAM_COUNTS() macro defines a max of 1000000, which is
> probably too low when you're logging bytes (i.e. I would expect you might get
> enough records above that, that it's worth using a higher max).
> 
> Probably, 100M is a good max to use (by reasoning of 10M still possibly being
> too low for some pages, so next magnitude).
> 
> Change these to:
> 
> const int kHistogramMin = 1;
> const int kHistogramMax = 100000000;  // 100M.
> const int kBucketCount = 50;
> 
> UMA_HISTOGRAM_CUSTOM_COUNTS("Prerender.NetworkBytes.Used", prerender_bytes,
>                             kHistogramMin, kHistogramMax, kBucketCount)

[Alexei Svitkine (slow), 2014-02-13 17:31:18]

lgtm

[jkarlin, 2014-02-13 19:51:56]

jochen: PTAL (mostly for owners) at the code outside of prerender/ and
histograms.xml  Thanks!

[jochen (gone - plz use gerrit), 2014-02-14 09:51:06]

lgtm

[jkarlin, 2014-02-14 11:57:20]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-02-14 11:57:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jkarlin@chromium.org/160513002/500001

[commit-bot: I haz the power, 2014-02-14 14:59:03]

Change committed as 251316