Add histograms to ResourceFetcher to measure memory cache hit rate.

Add histograms to ResourceFetcher to measure memory cache hit rate.

This will drive possible improvements that could be made in the cache to
decrease memory usage (at least on Android).

Note that the metrics are in the ResourceFetcher rather than in the MemoryCache
itself to only report events that really matter in practice (e.g. the resource
has to be fetched from the browser process vs. the resource was served directly
from the in-memory cache).

In addition to the overall hit rate, an important metric that this CL is adding
is whether a resource is live or dead upon a cache hit. This will help us
understand whether memory usage improvements can be made in the cache since
only dead resources can be evicted (live resources are used by clients thus
can't be deleted by definition).

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

[Philippe, 2013-09-13 11:38:26]

Hi Adam,

I would like to add these metrics to understand the current performance
characteristics of the resource cache before I consider tuning it for
memory-constrained devices.

[abarth-chromium, 2013-09-13 17:38:18]

Looks fine.  Several minor comments below.  I'd like to see one more iteration
before landing.

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource.h
File Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
Source/core/fetch/Resource.h:61: MainResource = 0,
This change doesn't do anything.

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
Source/core/fetch/Resource.h:74: TypeBoundaryValue = 13, // Used for histograms.
What is TypeBoundaryValue?  Perhaps you mean NumberOfTypes?

Also, no need for this comment.  Folks can grep the code to see where this value
is used.

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:75: namespace {
We usually have a blank line above anonymous namespaces.

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:77: // Used for histograms.
No need for this comment.

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:82: ActionUponResourceRequestBoundaryValue
= 3,
No need to list these numbers explicitly.

What is a boundary value?  How about NumberOfResourceRequestActions?

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:113: }
Why doesn't the compiler complain that you're missing an enum value in this
switch?

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:557:
"Renderer.ResourceFetcher.ActionUponResourceRequest", LoadResource,
Renderer -> WebCore

[Philippe, 2013-09-16 11:47:21]

Thanks Adam!

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource.h
File Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
Source/core/fetch/Resource.h:61: MainResource = 0,
On 2013/09/13 17:38:18, abarth wrote:
> This change doesn't do anything.

Done.

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
Source/core/fetch/Resource.h:74: TypeBoundaryValue = 13, // Used for histograms.
On 2013/09/13 17:38:18, abarth wrote:
> What is TypeBoundaryValue?  Perhaps you mean NumberOfTypes?
> 
> Also, no need for this comment.  Folks can grep the code to see where this
value
> is used.

Done.

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:75: namespace {
On 2013/09/13 17:38:18, abarth wrote:
> We usually have a blank line above anonymous namespaces.

Done.

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:77: // Used for histograms.
On 2013/09/13 17:38:18, abarth wrote:
> No need for this comment.

Done.

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:82: ActionUponResourceRequestBoundaryValue
= 3,
On 2013/09/13 17:38:18, abarth wrote:
> No need to list these numbers explicitly.
> 
> What is a boundary value?  How about NumberOfResourceRequestActions?

Boundary value is the terminology used in HistogramSupport.h. I agree it's not
very clear :)

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:113: }
On 2013/09/13 17:38:18, abarth wrote:
> Why doesn't the compiler complain that you're missing an enum value in this
> switch?

We don't provide any specific warning flags (on Linux/Android at least). The
only thing we do is to disable specific warning flags:
-Wno-unused-parameter, -Wno-missing-field-initializers, -Wno-format,
-Wno-unused-result, -Wno-c++0x-compat, -Wno-deprecated

As you can imagine adding -Wswitch-enum for non-Chromium code causes millions of
warning messages (turned into errors). We don't seem to distinguish Blink flags
from other third-party flags. I would expect that enabling certain compile flags
on Blink would be just as hard as on other third-party projects but
progressively enabling some of them in certain parts of the tree might make
sense now in my opinion since we own Blink. I expect that this would catch quite
a few potential bugs. For instance I remember once that I got bitten in Chromium
because I had forgotten to return a value in a non-void function (without even
any branch). This compiled just fine unfortunately.

This is slightly off-topic obviously :)

For this particular case I'm letting the ASSERT_NOT_REACHED() do the job on line
115 if you don't mind.

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:557:
"Renderer.ResourceFetcher.ActionUponResourceRequest", LoadResource,
On 2013/09/13 17:38:18, abarth wrote:
> Renderer -> WebCore

Done.

[Philippe, 2013-09-16 14:09:08]

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/23734006/diff/5001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:113: }
On 2013/09/16 11:47:21, Philippe wrote:
> On 2013/09/13 17:38:18, abarth wrote:
> > Why doesn't the compiler complain that you're missing an enum value in this
> > switch?
> 
> We don't provide any specific warning flags (on Linux/Android at least). The
> only thing we do is to disable specific warning flags:
> -Wno-unused-parameter, -Wno-missing-field-initializers, -Wno-format,
> -Wno-unused-result, -Wno-c++0x-compat, -Wno-deprecated
> 
> As you can imagine adding -Wswitch-enum for non-Chromium code causes millions
of
> warning messages (turned into errors). We don't seem to distinguish Blink
flags
> from other third-party flags. I would expect that enabling certain compile
flags
> on Blink would be just as hard as on other third-party projects but
> progressively enabling some of them in certain parts of the tree might make
> sense now in my opinion since we own Blink. I expect that this would catch
quite
> a few potential bugs. For instance I remember once that I got bitten in
Chromium
> because I had forgotten to return a value in a non-void function (without even
> any branch). This compiled just fine unfortunately.
> 
> This is slightly off-topic obviously :)
> 
> For this particular case I'm letting the ASSERT_NOT_REACHED() do the job on
line
> 115 if you don't mind.

Correction, the build does fail with Clang at least. So I added a new case where
it was needed.

[abarth-chromium, 2013-09-16 21:04:35]

I'm sad that this is making the code a bit uglier, but the alternatives appear
to be worse, so LGTM.

https://codereview.chromium.org/23734006/diff/18001/Source/core/fetch/Resource.h
File Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/23734006/diff/18001/Source/core/fetch/Resourc...
Source/core/fetch/Resource.h:74: NumberOfTypes = 13,
There's no need to assign a specific value to this name.  The compiler will give
it the right value automatically.

https://codereview.chromium.org/23734006/diff/18001/Source/core/fetch/Resourc...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/23734006/diff/18001/Source/core/fetch/Resourc...
Source/core/fetch/ResourceFetcher.cpp:82: NumberOfResourceRequestActions = 3,
ditto

https://codereview.chromium.org/23734006/diff/18001/Source/core/fetch/Resourc...
Source/core/fetch/ResourceFetcher.cpp:151: ASSERT_NOT_REACHED();
That's pretty ugly.

[Philippe, 2013-09-17 08:37:50]

Thanks Adam!

https://codereview.chromium.org/23734006/diff/18001/Source/core/fetch/Resource.h
File Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/23734006/diff/18001/Source/core/fetch/Resourc...
Source/core/fetch/Resource.h:74: NumberOfTypes = 13,
On 2013/09/16 21:04:35, abarth wrote:
> There's no need to assign a specific value to this name.  The compiler will
give
> it the right value automatically.

Yeah, I had kept this one to make it clear that this value has a special meaning
and should be kept last. Given that the values are not sorted it would be easy
for someone not to pay attention to this value and add a new value at the end
after this one. But I removed it anyway.

https://codereview.chromium.org/23734006/diff/18001/Source/core/fetch/Resourc...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/23734006/diff/18001/Source/core/fetch/Resourc...
Source/core/fetch/ResourceFetcher.cpp:82: NumberOfResourceRequestActions = 3,
On 2013/09/16 21:04:35, abarth wrote:
> ditto

Done.

https://codereview.chromium.org/23734006/diff/18001/Source/core/fetch/Resourc...
Source/core/fetch/ResourceFetcher.cpp:151: ASSERT_NOT_REACHED();
On 2013/09/16 21:04:35, abarth wrote:
> That's pretty ugly.

Yeah I know :) I didn't want to make this a default case since it would have
allowed developers adding a new value to the enum to ignore these switch
statements.

[commit-bot: I haz the power, 2013-09-17 08:38:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/23734006/25001

[commit-bot: I haz the power, 2013-09-17 09:38:54]

Change committed as 157890