Add histogram to measure the read deferral on crosswebsite navigation

Add histogram to measure the read deferral on crosswebsite navigation

On crosswebsite navigation, after receiving and validating the response
headers, read of the response body is deferred. This histogram measure this
deferral.

BUG=

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

[pasko, 2013-05-21 16:37:51]

I think having this metric would be super useful to see how much time we are
spending in unload activities and also how often this deferral happens.

I'd recommend adding simonjam@, luckily he is from OWNERS.

https://chromiumcodereview.appspot.com/15211004/diff/1/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://chromiumcodereview.appspot.com/15211004/diff/1/content/browser/loader...
content/browser/loader/resource_loader.cc:368: compute_read_deferral_histogram_
= true;
I think reflecting the state rather than intention would be better:
"deferred_before_initial_resume_ = true;".

https://chromiumcodereview.appspot.com/15211004/diff/1/content/browser/loader...
content/browser/loader/resource_loader.cc:369: deferral_time_ =
base::TimeTicks::Now();
this would sound more logical with the name response_started_time_

https://chromiumcodereview.appspot.com/15211004/diff/1/content/browser/loader...
content/browser/loader/resource_loader.cc:595:
UMA_HISTOGRAM_TIMES("Net.ResourceLoader.ReadDeferral",
please also add a pairing change in:
tools/metrics/histograms/histograms.xml

Would require adding jar@ as reviewer for that part.

[pasko-google - do not use, 2013-05-29 15:50:34]

a friendly ping :)

[pasko, 2013-08-01 15:51:05]

Camille, James, can we land something like that before M30 please?

[James Simonsen, 2013-08-01 19:17:40]

https://codereview.chromium.org/15211004/diff/1/content/browser/loader/resour...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/15211004/diff/1/content/browser/loader/resour...
content/browser/loader/resource_loader.cc:368: compute_read_deferral_histogram_
= true;
I don't think you need this bool at all. Just check deferral_time_.is_null().

https://codereview.chromium.org/15211004/diff/1/content/browser/loader/resour...
content/browser/loader/resource_loader.cc:564: deferred_stage_ = DEFERRED_READ; 
// Read first chunk when resumed.
Seems like it'd make more sense to set deferral_time_ here.

https://codereview.chromium.org/15211004/diff/1/content/browser/loader/resour...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/15211004/diff/1/content/browser/loader/resour...
content/browser/loader/resource_loader.h:131: base::TimeTicks deferral_time_;
read_deferral_start_time_?

[clamy, 2013-08-05 14:33:44]

jar@: please take a look at the changes in histograms.xml.

https://chromiumcodereview.appspot.com/15211004/diff/1/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://chromiumcodereview.appspot.com/15211004/diff/1/content/browser/loader...
content/browser/loader/resource_loader.cc:368: compute_read_deferral_histogram_
= true;
On 2013/08/01 19:17:40, James Simonsen wrote:
> I don't think you need this bool at all. Just check deferral_time_.is_null().

Done.

https://chromiumcodereview.appspot.com/15211004/diff/1/content/browser/loader...
content/browser/loader/resource_loader.cc:564: deferred_stage_ = DEFERRED_READ; 
// Read first chunk when resumed.
On 2013/08/01 19:17:40, James Simonsen wrote:
> Seems like it'd make more sense to set deferral_time_ here.

Done.

https://chromiumcodereview.appspot.com/15211004/diff/1/content/browser/loader...
File content/browser/loader/resource_loader.h (right):

https://chromiumcodereview.appspot.com/15211004/diff/1/content/browser/loader...
content/browser/loader/resource_loader.h:131: base::TimeTicks deferral_time_;
On 2013/08/01 19:17:40, James Simonsen wrote:
> read_deferral_start_time_?

Done.

[James Simonsen, 2013-08-05 16:50:58]

lgtm

[clamy, 2013-08-07 11:53:26]

isherman@: Please take a look at the changes in histograms.xml

[Ilya Sherman, 2013-08-08 07:33:21]

histograms.xml lgtm

[commit-bot: I haz the power, 2013-08-08 10:13:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clamy@chromium.org/15211004/15001

[commit-bot: I haz the power, 2013-08-09 09:48:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/clamy@chromium.org/15211004/15001

[commit-bot: I haz the power, 2013-08-09 11:05:25]

Change committed as 216660