Add histograms to ResourceFetcher to evaluate memory cache hit rate

Add histograms to ResourceFetcher to evaluate memory cache hit rate

This is a rework of https://chromiumcodereview.appspot.com/23734006 that was
reverted due to a regression in PLT. This patch introduces three histograms
that measures the count of dead resources used from cache, revalidated, or
loaded again. This will drive possible improvements that could be made to the
cache to  decrease memory usage on Android.

BUG=290193
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166121

[clamy, 2014-01-28 12:29:53]

abarth, pliard: PTAL

This is an attempt at relanding the metrics for hit rate in the ResourceFetcher
that Phillipe landed last September (and which caused a regression in PLT).

[abarth-chromium, 2014-01-28 20:18:09]

+japhet

[Nate Chapin, 2014-01-28 20:22:54]

https://chromiumcodereview.appspot.com/148323002/diff/40001/Source/core/fetch...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://chromiumcodereview.appspot.com/148323002/diff/40001/Source/core/fetch...
Source/core/fetch/ResourceFetcher.cpp:642: ++m_loadCount;
Would it be cleaner to wrap these variables up into a simple helper class owner
by ResourceFetcher?

I'd at least prefer that incrementing the appropriate counter be done in a
helper function, rather than in this switch statement.

[clamy, 2014-01-29 12:19:20]

https://chromiumcodereview.appspot.com/148323002/diff/40001/Source/core/fetch...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://chromiumcodereview.appspot.com/148323002/diff/40001/Source/core/fetch...
Source/core/fetch/ResourceFetcher.cpp:642: ++m_loadCount;
On 2014/01/28 20:22:54, Nate Chapin wrote:
> Would it be cleaner to wrap these variables up into a simple helper class
owner
> by ResourceFetcher?
> 
> I'd at least prefer that incrementing the appropriate counter be done in a
> helper function, rather than in this switch statement.

Done.

[Nate Chapin, 2014-01-29 18:19:56]

LGTM w/nits

https://chromiumcodereview.appspot.com/148323002/diff/80001/Source/core/fetch...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://chromiumcodereview.appspot.com/148323002/diff/80001/Source/core/fetch...
Source/core/fetch/ResourceFetcher.cpp:635: if (!resource->hasClients())
We null-check resource just below this. Either this should be after the
null-check, or we should remove the null-check. I'm pretty sure it's
unnecessary.

https://chromiumcodereview.appspot.com/148323002/diff/80001/Source/core/fetch...
Source/core/fetch/ResourceFetcher.cpp:1400: // Fall through.
Given that Reload and Load cases are identical, this comment probably isn't
necessary.

https://chromiumcodereview.appspot.com/148323002/diff/80001/Source/core/fetch...
Source/core/fetch/ResourceFetcher.cpp:1403: break;
return instead?

[commit-bot: I haz the power, 2014-01-30 10:27:38]

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

[commit-bot: I haz the power, 2014-01-30 11:32:57]

Change committed as 166121

[commit-bot: I haz the power, 2014-01-30 11:33:03]

CQ bit was unchecked on CL. Ignoring.