Http cache: Measure back navigations on the infinite cache simulation.

Http cache: Measure back navigations on the infinite cache simulation.

Also count entries that are not cached.

BUG=147383
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=164465

[gavinp, 2012-10-25 18:25:47]

Is this two CLs together?

http://codereview.chromium.org/11267028/diff/1/net/http/infinite_cache.cc
File net/http/infinite_cache.cc (right):

http://codereview.chromium.org/11267028/diff/1/net/http/infinite_cache.cc#new...
net/http/infinite_cache.cc:538:
UMA_HISTOGRAM_BOOLEAN("InfiniteCache.DoomMethodHit", true);
Is this part of a separate CL that got mixed in here? This looks more like ways
to get apples-to-apples counts?

http://codereview.chromium.org/11267028/diff/1/net/http/infinite_cache.h
File net/http/infinite_cache.h (right):

http://codereview.chromium.org/11267028/diff/1/net/http/infinite_cache.h#newc...
net/http/infinite_cache.h:47: void OnBackNavigation();
Nit: say either History navigation or BackForward here.

[rvargas (doing something else), 2012-10-25 18:32:18]

http://codereview.chromium.org/11267028/diff/1/net/http/infinite_cache.cc
File net/http/infinite_cache.cc (right):

http://codereview.chromium.org/11267028/diff/1/net/http/infinite_cache.cc#new...
net/http/infinite_cache.cc:538:
UMA_HISTOGRAM_BOOLEAN("InfiniteCache.DoomMethodHit", true);
On 2012/10/25 18:25:47, gavinp wrote:
> Is this part of a separate CL that got mixed in here? This looks more like
ways
> to get apples-to-apples counts?

part of this CL. The whole CL is about getting closer to apples to apples... and
get a better picture of where requests go.

http://codereview.chromium.org/11267028/diff/1/net/http/infinite_cache.h
File net/http/infinite_cache.h (right):

http://codereview.chromium.org/11267028/diff/1/net/http/infinite_cache.h#newc...
net/http/infinite_cache.h:47: void OnBackNavigation();
On 2012/10/25 18:25:47, gavinp wrote:
> Nit: say either History navigation or BackForward here.

Done.

[gavinp, 2012-10-25 18:56:26]

The more I read the code, the more I'm struck by how similar the HttpCache
pattern histograms are to the infinite cache.

Do you think the path to direct apples to apples is to make those directly
comparable?

http://codereview.chromium.org/11267028/diff/1/net/http/infinite_cache.cc
File net/http/infinite_cache.cc (right):

http://codereview.chromium.org/11267028/diff/1/net/http/infinite_cache.cc#new...
net/http/infinite_cache.cc:538:
UMA_HISTOGRAM_BOOLEAN("InfiniteCache.DoomMethodHit", true);
On 2012/10/25 18:32:18, rvargas wrote:
> On 2012/10/25 18:25:47, gavinp wrote:
> > Is this part of a separate CL that got mixed in here? This looks more like
> ways
> > to get apples-to-apples counts?
> 
> part of this CL. The whole CL is about getting closer to apples to apples...
and
> get a better picture of where requests go.

Great. The CL probably needs the subject and description updated, too.

http://codereview.chromium.org/11267028/diff/8002/net/http/infinite_cache.cc
File net/http/infinite_cache.cc (left):

http://codereview.chromium.org/11267028/diff/8002/net/http/infinite_cache.cc#...
net/http/infinite_cache.cc:573: return InitializeData();;
nice.

http://codereview.chromium.org/11267028/diff/8002/net/http/infinite_cache.cc
File net/http/infinite_cache.cc (right):

http://codereview.chromium.org/11267028/diff/8002/net/http/infinite_cache.cc#...
net/http/infinite_cache.cc:281:
UMA_HISTOGRAM_BOOLEAN("InfiniteCache.BackNavigation", true);
Back --> BackForward here too

http://codereview.chromium.org/11267028/diff/8002/net/http/infinite_cache.cc#...
net/http/infinite_cache.cc:575:
UMA_HISTOGRAM_BOOLEAN("InfiniteCache.DoomMethodMiss", true);
Why not report hits and misses in the same histogram? Then use an enumeration
type in the xml file, and the dashboard will have one histogram showing HITS and
MISS together.

enum { DOOM_HIT, DOOM_MISS, DOOM_MAX };

UMA_HISTOGRAM_ENUMERATION("InfiniteCache.DoomMethod", DOOM_HIT, DOOM_MAX);

(and the same above)

[rvargas (doing something else), 2012-10-25 19:04:49]

http://codereview.chromium.org/11267028/diff/1/net/http/infinite_cache.cc
File net/http/infinite_cache.cc (right):

http://codereview.chromium.org/11267028/diff/1/net/http/infinite_cache.cc#new...
net/http/infinite_cache.cc:538:
UMA_HISTOGRAM_BOOLEAN("InfiniteCache.DoomMethodHit", true);
On 2012/10/25 18:56:26, gavinp wrote:
> On 2012/10/25 18:32:18, rvargas wrote:
> > On 2012/10/25 18:25:47, gavinp wrote:
> > > Is this part of a separate CL that got mixed in here? This looks more like
> > ways
> > > to get apples-to-apples counts?
> > 
> > part of this CL. The whole CL is about getting closer to apples to apples...
> and
> > get a better picture of where requests go.
> 
> Great. The CL probably needs the subject and description updated, too.

Feel free to modify it as you wish.

http://codereview.chromium.org/11267028/diff/8002/net/http/infinite_cache.cc
File net/http/infinite_cache.cc (right):

http://codereview.chromium.org/11267028/diff/8002/net/http/infinite_cache.cc#...
net/http/infinite_cache.cc:281:
UMA_HISTOGRAM_BOOLEAN("InfiniteCache.BackNavigation", true);
On 2012/10/25 18:56:26, gavinp wrote:
> Back --> BackForward here too

Done.

http://codereview.chromium.org/11267028/diff/8002/net/http/infinite_cache.cc#...
net/http/infinite_cache.cc:575:
UMA_HISTOGRAM_BOOLEAN("InfiniteCache.DoomMethodMiss", true);
On 2012/10/25 18:56:26, gavinp wrote:
> Why not report hits and misses in the same histogram? Then use an enumeration
> type in the xml file, and the dashboard will have one histogram showing HITS
and
> MISS together.
> 
> enum { DOOM_HIT, DOOM_MISS, DOOM_MAX };
> 
> UMA_HISTOGRAM_ENUMERATION("InfiniteCache.DoomMethod", DOOM_HIT, DOOM_MAX);
> 
> (and the same above)

because it is more complicated? It also means that we need to merge the
codepaths to get to a single point where to add the histogram.

[rvargas (doing something else), 2012-10-25 19:15:41]

On 2012/10/25 18:56:26, gavinp wrote:
> The more I read the code, the more I'm struck by how similar the HttpCache
> pattern histograms are to the infinite cache.
> 
> Do you think the path to direct apples to apples is to make those directly
> comparable?

I forgot this part... Sorry, I don't know what you mean. I don't plan to make
the two values comparable... just getting closer to compare the disk cache HR
with the infinite cache HR, but even that is not going to work very well (and
I'm not convinced of the value proposition of that goal at this point).

[gavinp, 2012-10-25 20:09:48]

On 2012/10/25 19:04:49, rvargas wrote:
> http://codereview.chromium.org/11267028/diff/1/net/http/infinite_cache.cc
> File net/http/infinite_cache.cc (right):
> 
>
http://codereview.chromium.org/11267028/diff/1/net/http/infinite_cache.cc#new...
> net/http/infinite_cache.cc:538:
> UMA_HISTOGRAM_BOOLEAN("InfiniteCache.DoomMethodHit", true);
> On 2012/10/25 18:56:26, gavinp wrote:
> > On 2012/10/25 18:32:18, rvargas wrote:
> > > On 2012/10/25 18:25:47, gavinp wrote:
> > > > Is this part of a separate CL that got mixed in here? This looks more
like
> > > ways
> > > > to get apples-to-apples counts?
> > > 
> > > part of this CL. The whole CL is about getting closer to apples to
apples...
> > and
> > > get a better picture of where requests go.
> > 
> > Great. The CL probably needs the subject and description updated, too.
> 
> Feel free to modify it as you wish.
> 
> http://codereview.chromium.org/11267028/diff/8002/net/http/infinite_cache.cc
> File net/http/infinite_cache.cc (right):
> 
>
http://codereview.chromium.org/11267028/diff/8002/net/http/infinite_cache.cc#...
> net/http/infinite_cache.cc:281:
> UMA_HISTOGRAM_BOOLEAN("InfiniteCache.BackNavigation", true);
> On 2012/10/25 18:56:26, gavinp wrote:
> > Back --> BackForward here too
> 
> Done.
> 
>
http://codereview.chromium.org/11267028/diff/8002/net/http/infinite_cache.cc#...
> net/http/infinite_cache.cc:575:
> UMA_HISTOGRAM_BOOLEAN("InfiniteCache.DoomMethodMiss", true);
> On 2012/10/25 18:56:26, gavinp wrote:
> > Why not report hits and misses in the same histogram? Then use an
enumeration
> > type in the xml file, and the dashboard will have one histogram showing HITS
> and
> > MISS together.
> > 
> > enum { DOOM_HIT, DOOM_MISS, DOOM_MAX };
> > 
> > UMA_HISTOGRAM_ENUMERATION("InfiniteCache.DoomMethod", DOOM_HIT, DOOM_MAX);
> > 
> > (and the same above)
> 
> because it is more complicated? It also means that we need to merge the
> codepaths to get to a single point where to add the histogram.

It's not necessary to only add histograms at a single point, and the cost in
memory would actually be lower to use the same histogram twice in two places
than to have two different histograms.

If you don't like the enum, then consider reporting a true/false bool, but then
in the XML make it an enumeration type named HIT and MISS, then we'll get more
compact reporting on the dashboard.

[rvargas (doing something else), 2012-10-25 20:44:45]

> It's not necessary to only add histograms at a single point, and the cost in

Last time I checked it was a requirement to have a single place for a histogram.
Attempting to register the same histogram twice was a mistake. Has that changed?

> memory would actually be lower to use the same histogram twice in two places
> than to have two different histograms.

I expect the memory cost of a boolean histogram to be really low... sure, it is
slightly more costly to have two boolean histograms than a histogram for an enum
a few values, but I doubt that's the deciding factor in this case.



> 
> If you don't like the enum, then consider reporting a true/false bool, but
then
> in the XML make it an enumeration type named HIT and MISS, then we'll get more
> compact reporting on the dashboard.

[rvargas (doing something else), 2012-10-26 21:02:27]

OK... Now I'm using the same histogram :)

[gavinp, 2012-10-26 22:06:14]

LGTM, especially since I know we want improved metrics in the next branch cut. I
wish I had more context so I knew what the ultimate apples-to-apples goal was
though.

There's probably a not very hath to apples to apples comparisons by using the
HttpCache::Transaction::Pattern logic... The InfiniteCacheTransaction could
perhaps link the two reports, so we'd get something that would match more
closely.

Earlier you said you didn't think an apples to apples comparison was worth
doing, though. Why? What makes it not good?

http://codereview.chromium.org/11267028/diff/8004/net/http/http_cache_transac...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/11267028/diff/8004/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:268:
infinite_cache_transaction_->OnBackForwardNavigation();
Oops: I made this comment on an earlier version but somehow it didn't get
uploaded. Have you considered reporting an enumeration giving which of these
flags was set, plus an extra bonus "NONE" field?

This is just a take it or leave it suggestion.

[rvargas (doing something else), 2012-10-26 22:36:12]

There's not much more context to the apples to apples thing. We know that the
real disk cache HR is about 8 points higher than what the simulation is telling
us so this CL is about exploring the hypothesis that most of that difference
comes from back navigations.

I don't really want to have the simulation perform all the logic that the HTTP
and Disk caches perform, or to by tied to a bunch of places everywhere to
attempt to follow the same logic. I want to keep the simple model of "this is a
request, this is the response" that we have today, with just the minimum http
logic embedded on the simulation to have a very simple, general http cache.

This basically implies that the values won't be directly comparable: the real
version will act based on all flag and header combinations, will do something
different depending on the response being cached or not etc. The numbers will
never match (unless the real cache is also infinite, and we add all the
reporting logic to the real one).

I said that I'm not convinced that this is the right thing to do because the
number will go up, but we'll still be unable to compare them. Now we won't have
page reloads, that may end up removing stuff from the cache (or at the very
least affecting the validation)... so should the HR be higher or lower?

And the final point is that even though the whole experiment was designed to
measure HR (tons of them), as a proxy for cache performance, I'm once again
convinced that this is a bogus measurement (the reason I have not payed
attention to HR for years). That is not to say that nothing is coming out of the
simulation.

Thanks

http://codereview.chromium.org/11267028/diff/8004/net/http/http_cache_transac...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/11267028/diff/8004/net/http/http_cache_transac...
net/http/http_cache_transaction.cc:268:
infinite_cache_transaction_->OnBackForwardNavigation();
On 2012/10/26 22:06:14, gavinp wrote:
> Oops: I made this comment on an earlier version but somehow it didn't get
> uploaded. Have you considered reporting an enumeration giving which of these
> flags was set, plus an extra bonus "NONE" field?
> 
> This is just a take it or leave it suggestion.

Kind of. I considered passing effective_load_flags_ to the simulation and having
histograms for other stuff, but I decided it was better not to do that.

This is at a layer that is not exactly what concerns the simulation (and the
same thing can be said about sending back navigations, but we want to be able to
compare that number with the people on the experiment).

What I mean is that the purpose of the simulation is to measure the behavior of
a very big cache, and the percentage of request issued with a particular flag
doesn't have anything to do with having a big cache or not, so if we really care
about that we could just as well add a regular histogram somewhere else.