Disk Cache: Add more corruption tracking histograms.

Disk Cache: Add more corruption tracking histograms.

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134678

[rvargas (doing something else), 2012-04-19 21:45:09]

http://codereview.chromium.org/10148001/diff/1/net/disk_cache/rankings.cc
File net/disk_cache/rankings.cc (right):

http://codereview.chromium.org/10148001/diff/1/net/disk_cache/rankings.cc#new...
net/disk_cache/rankings.cc:853: int Rankings::CheckListSection(List list, Addr
stop1, Addr stop2, bool forward,
I know you don't like boolean arguments, but this is a private method, called
from a single method (and I'm overly explicit when making the call)

[gavinp, 2012-04-23 19:10:37]

You predicted my nit right.

Not related to this CL, but food for thought: rankings.cc is now over half
consistency checking code.  Should we consolidate all these consistency checks
in a separate helper class?

http://codereview.chromium.org/10148001/diff/1/net/disk_cache/rankings.cc
File net/disk_cache/rankings.cc (right):

http://codereview.chromium.org/10148001/diff/1/net/disk_cache/rankings.cc#new...
net/disk_cache/rankings.cc:792: forward = false;
forward = false is confusing.  You're right that I don't like bool params.  I
think this would be much more readable if in Rankings there was an enum {
CHECK_FORWARDS, CHECK_BACKWARDS };

[rvargas (doing something else), 2012-04-23 21:22:25]

thanks

http://codereview.chromium.org/10148001/diff/1/net/disk_cache/rankings.cc
File net/disk_cache/rankings.cc (right):

http://codereview.chromium.org/10148001/diff/1/net/disk_cache/rankings.cc#new...
net/disk_cache/rankings.cc:792: forward = false;
On 2012/04/23 19:10:37, gavinp wrote:
> forward = false is confusing.

why? What would be an appropriate name for a bool variable that says if the
direction to search is forward or not? is_forward ?

> think this would be much more readable if in Rankings there was an enum {
> CHECK_FORWARDS, CHECK_BACKWARDS };

What I don't like about this (other than the nth redefinition of 0 and 1) is
that the enum has to be declared on the header file: noise for everybody instead
of being something confined to the cc file, that nobody will see unless they are
reading the one function that cares about it.

[gavinp, 2012-04-25 12:40:23]

If you don't want to put the enum in the header, then I think the best way is
probably to move these check functions into an anon namespace in the .cc file,
or move them into their own utility files.  Either way we're not polluting the
header like you're worried.

http://codereview.chromium.org/10148001/diff/1/net/disk_cache/rankings.cc
File net/disk_cache/rankings.cc (right):

http://codereview.chromium.org/10148001/diff/1/net/disk_cache/rankings.cc#new...
net/disk_cache/rankings.cc:792: forward = false;
is_forward is definitely a better name.

But I still think the enum is the way to go.  The language is already full of
ways to stay the same number two ways; that argument is unpersuasive.

I hear you about the header file; but I think the result will be that the
signature of CheckListSection is easier to read; and really, folks aren't trying
to read through the private section of an header file unless they wish to
understand a lot of private interface.

But why not move CheckList and CheckListSection into an anon namespace?  It
seems that CheckList only derefs this to call CheckListSection, and
CheckListSection defefs this for heads_ and tails_.  If you added those as
params, you could move these to an anon namespace, and the enum would be in this
file.

On 2012/04/23 21:22:26, rvargas wrote:
> On 2012/04/23 19:10:37, gavinp wrote:
> > forward = false is confusing.
> 
> why? What would be an appropriate name for a bool variable that says if the
> direction to search is forward or not? is_forward ?
> 
> > think this would be much more readable if in Rankings there was an enum {
> > CHECK_FORWARDS, CHECK_BACKWARDS };
> 
> What I don't like about this (other than the nth redefinition of 0 and 1) is
> that the enum has to be declared on the header file: noise for everybody
instead
> of being something confined to the cc file, that nobody will see unless they
are
> reading the one function that cares about it.

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

On 2012/04/25 12:40:23, gavinp wrote:
> If you don't want to put the enum in the header, then I think the best way is
> probably to move these check functions into an anon namespace in the .cc file,
> or move them into their own utility files.  Either way we're not polluting the
> header like you're worried.

Moving them to a helper file means that it will become a public interface, and
that means concerns about replacing a boolean with an enum would only be
stronger.

Moving them to a namespace is doable (although there are more dependencies than
heads and tails), but then we'd need to move more things to arguments, and
honestly, I think these are not really utility functions: they are the
responsibility of the class that maintains the lists (rankings). As much as
inserting is one of the class chores, verifying if something is properly
inserted should also be provided by the class (otherwise, internal details have
to be shared between the inserter and the check_for_insertion).

I'm about to give up on this argument, but I want to point out that I went over
the style guide again and having bool arguments is not disallowed. In fact,
there are examples about how to write the caller to make things clearer for the
reader, and IMO there is absolutely no gain from reading a header where it says
that there is a boolean argument that controls the direction to use vs reading
the definition of an enumeration with two values and later on finding a method
that uses that enumeration (in fact, it is worse in that it forces me back to
look for the definition of that enumeration, because now it is not evident that
it can only be forward/backward without reading the actual definition... there
will be 39 lines between the declaration and the single use of this enum).

So no, I don't agree that having an enum is better for the API, but now we have
yet another  enum.

> 
> http://codereview.chromium.org/10148001/diff/1/net/disk_cache/rankings.cc
> File net/disk_cache/rankings.cc (right):
> 
>
http://codereview.chromium.org/10148001/diff/1/net/disk_cache/rankings.cc#new...
> net/disk_cache/rankings.cc:792: forward = false;
> is_forward is definitely a better name.
> 
> But I still think the enum is the way to go.  The language is already full of
> ways to stay the same number two ways; that argument is unpersuasive.

I don't mind if the language allows me to express the same thing in different
ways... I'm talking about having dozens of extra definitions on the symbol files
for the same thing (and all over the code, to be honest).


> 
> I hear you about the header file; but I think the result will be that the
> signature of CheckListSection is easier to read; and really, folks aren't
trying
> to read through the private section of an header file unless they wish to
> understand a lot of private interface.
> 
> But why not move CheckList and CheckListSection into an anon namespace?  It
> seems that CheckList only derefs this to call CheckListSection, and
> CheckListSection defefs this for heads_ and tails_.  If you added those as
> params, you could move these to an anon namespace, and the enum would be in
this
> file.
> 
> On 2012/04/23 21:22:26, rvargas wrote:
> > On 2012/04/23 19:10:37, gavinp wrote:
> > > forward = false is confusing.
> > 
> > why? What would be an appropriate name for a bool variable that says if the
> > direction to search is forward or not? is_forward ?
> > 
> > > think this would be much more readable if in Rankings there was an enum {
> > > CHECK_FORWARDS, CHECK_BACKWARDS };
> > 
> > What I don't like about this (other than the nth redefinition of 0 and 1) is
> > that the enum has to be declared on the header file: noise for everybody
> instead
> > of being something confined to the cc file, that nobody will see unless they
> are
> > reading the one function that cares about it.

[gavinp, 2012-04-26 15:46:54]

I went through this patch a lot more carefully, and unfortunately, I found a lot
more readability issues; my apologies for not catching  them earlier, but I
guess the bool caught my attention and I sent a comment before doing a thorough
reading.... I know that's dragging out your commit.  I apologise. I should have
done all readability issues in parallel.

A namespace or a separate class is doable if you want to cut the size of this
interface in rankings.h, and this would also be a great case for the use of
friend if you didn't want to carry around the large parameter lists.  Logically
checking does make sense; but the checkers have to be readable.  I spent over an
hour this morning reading this code before I realised last3 and last4 mean
second and first?  Or do they?  Maybe I'm just bad at reading code, but I
couldn't understand this checker very well.

CheckList() references heads_, tails_, control_data_, count_lists_ and for the
cache-specific UMA, backend_.

CheckListSection() references heads_, tails_ and for the cache-specific UMA,
backend_.

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/entry_impl.cc
File net/disk_cache/entry_impl.cc (right):

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/entry_impl....
net/disk_cache/entry_impl.cc:580: if (!rankings_addr.SanityCheckForRankings())
This is really good: the new function call makes what's happening a lot clearer
to a reader at the same time that it adds functionality.

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/errors.h
File net/disk_cache/errors.h (right):

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/errors.h#ne...
net/disk_cache/errors.h:13: enum {
NO_ERROR = 0,

or, to be more consistant with the rest of net:

OK = 0,

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc
File net/disk_cache/rankings.cc (right):

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:789: Addr last3, last4;
Don't we mean "first" and "second" here?

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:790: int num_items2;
num_items_backwards ?

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:804: // There's no expected value, so we'll use
something else.
So, if the second element is the last element or the second last element, or the
first element is the last or second last, we expected one more than we had? 
Why?

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:812:
UMA_HISTOGRAM_CUSTOM_COUNTS("DiskCache.LostItems",
So if I understand the comment, the range is  -9 to +8.   The name says it's the
number of lost items.  The call to HISTOGRAM_CUSTOM_COUNTS puts the minimum as
0, which is histogramspeak for 1, which is really 0 since all underflow will be
0.  The maximum is given as 2000, and 75 buckets are allocated, even though we
just said we only want 18.

UMA_HISTOGRAM_ENUMERATION("DiskCache.LostItemsPlus10", expected - total_items +
10, 18);  // comment justifying 18.

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:825: int error = list * 10;
10?  And not 9?

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:843: CACHE_UMA(CACHE_ERROR, "ListError", 0, error);
"ListTypeAndError" ??

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:871: CacheAddr next = (dir == FORWARD) ?
node->Data()->next : node->Data()->prev;
This is really pessimal, including dir == FORWARD everywhere.  I realise it was
a bool, and I pushed hard for the parameter to be a bool, but why not have it
both ways?

const bool is_forward = (dir == FORWARD); 

hen we have an interface that makes callers readable, and usage is readable.

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:894: return 0;
return OK;

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.h
File net/disk_cache/rankings.h (right):

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.h#...
net/disk_cache/rankings.h:149: enum ListDirection {
Maybe name this ForwardsOrBackwards ?

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.h#...
net/disk_cache/rankings.h:184: // Walks a list in the desired direction until
|stop1| or |stop2| nodes are
I like "end", as it's used in the standard library to mean the same concept a
number of times.  I don't like these number postfixes; especially since in the
same function declaration we use the same pattern of postfixing to mean two
completely different things (ordering vs alternatives).

Also, a grammar nit:
until the nodes |end| or |end_other| are reached.

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.h#...
net/disk_cache/rankings.h:186: // and the addresses of the last nodes visited
(|last2| was seen before
how about |second_last| and |last|?

[rvargas (doing something else), 2012-04-27 00:18:10]

I apologize for the time lost following the CL.

I understand it may look deceivingly short for what it does, but I didn't though
it was hard to follow once finished. (except for the mapping of errors, but that
seems to be fine judging by the lack of comments).

In fact, moving the loop to a dedicated method was meant to reduce CheckList to
mostly boilerplate code, so I'm a little lost because most issues seem to be on
that code. Would a longer description of the code help?

I still don't think moving the checkers to another class is the right move... I
don't see that improving the readability of the code, and as you point out,
access to more than half of the members is required (and while friending is
possible, I believe that a class should have as many members as needed, and a
function as many lines as required... and this class is not particularly big)

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/errors.h
File net/disk_cache/errors.h (right):

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/errors.h#ne...
net/disk_cache/errors.h:13: enum {
On 2012/04/26 15:46:54, gavinp wrote:
> NO_ERROR = 0,
> 
> or, to be more consistant with the rest of net:
> 
> OK = 0,

I don't want to give the impression that the errors are the same (or that one
result can be just forwarded to code expecting the other).

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc
File net/disk_cache/rankings.cc (right):

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:789: Addr last3, last4;
On 2012/04/26 15:46:54, gavinp wrote:
> Don't we mean "first" and "second" here?

I'm sorry I don't follow you here. What is first and second?. This is the third
and fourth values that this function receives from calling CheckListSection...
so we have up to two nodes seen in each direction, but this code really doesn't
have a need (so far) to differentiate between them. We may need that in the
future if we attempt to fix the list.

So that's what I have four generic names, because they are just four generic
nodes.

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:804: // There's no expected value, so we'll use
something else.
On 2012/04/26 15:46:54, gavinp wrote:
> So, if the second element is the last element or the second last element, or
the
> first element is the last or second last, we expected one more than we had? 
> Why?

Not an exact science, but if the two searches overlap, it means that we are not
missing anything... but there is at least one item that we'll discard
(otherwise, there should be no error). If the searches don't overlap we have no
idea how many elements are missing.

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:812:
UMA_HISTOGRAM_CUSTOM_COUNTS("DiskCache.LostItems",
On 2012/04/26 15:46:54, gavinp wrote:
> So if I understand the comment, the range is  -9 to +8.   The name says it's
the
> number of lost items.  The call to HISTOGRAM_CUSTOM_COUNTS puts the minimum as
> 0, which is histogramspeak for 1, which is really 0 since all underflow will
be
> 0.  The maximum is given as 2000, and 75 buckets are allocated, even though we
> just said we only want 18.
> 
> UMA_HISTOGRAM_ENUMERATION("DiskCache.LostItemsPlus10", expected - total_items
+
> 10, 18);  // comment justifying 18.

Nope. IMO, the histogram ranges (log ones) are kind of broken, and it boils down
to "try something until it looks like what you want". What I want is a log scale
with enough range to see if we miss 1000 items on a list, to see if we miss the
stop markers (even though we have two to reduce that chance), and get a
reasonable resolution around 0.

The comment states that the range is linear from -9 to +8.. that means that all
buckets are storing just one value... -10 (aka 0) is the negative overflow, and
I don't really know what it means... probably that the counter is wrong, maybe
that we really missed the markers. From 19 to 2000 we have a log scale... and we
need 75 buckets in order to have that many linear buckets close to 0.

From the point of view of seeing results, it's quite easy: just subtract 10 from
the bucket that you're looking at.

Note that the actual histogram description is where the "user" should know
what's being measured.

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:825: int error = list * 10;
On 2012/04/26 15:46:54, gavinp wrote:
> 10?  And not 9?

I'm going for something that is easier to read (when looking at the histogram),
so decimal system works better. This is just a standard "packed" value, in
decimal.

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:843: CACHE_UMA(CACHE_ERROR, "ListError", 0, error);
On 2012/04/26 15:46:54, gavinp wrote:
> "ListTypeAndError" ??

ListType sounds unrelated to ListError, so ListTypeAndError doesn't tell me at
all what's being displayed... not that it really matters, because I have to read
the description to know what it's about. But I'd always forget to look under
"Type" when looking for "Error"

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:871: CacheAddr next = (dir == FORWARD) ?
node->Data()->next : node->Data()->prev;
This is getting ridiculous. The natural value is bool, not an enum... if you
really want an enum, then live with it. I strongly believe that the previous
version is better than this one, and having even more noise around to try and
fix the issues introduced by that decision is not "the right thing to do".

On 2012/04/26 15:46:54, gavinp wrote:
> This is really pessimal, including dir == FORWARD everywhere.  I realise it
was
> a bool, and I pushed hard for the parameter to be a bool, but why not have it
> both ways?
> 
> const bool is_forward = (dir == FORWARD); 
> 
> hen we have an interface that makes callers readable, and usage is readable.

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.h
File net/disk_cache/rankings.h (right):

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.h#...
net/disk_cache/rankings.h:149: enum ListDirection {
On 2012/04/26 15:46:54, gavinp wrote:
> Maybe name this ForwardsOrBackwards ?

The name of a type should express what the type is, not what the possible values
of that type are.

I think this is you unconsciously looking for a bool :p

[gavinp, 2012-04-30 18:57:15]

OK, you've worn me down on the bool.... But I'm still thinking last3, last4
could change.

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc
File net/disk_cache/rankings.cc (right):

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:789: Addr last3, last4;
If I'm moving backwards, then when I reach the end, isn't that the beginning?

On 2012/04/27 00:18:10, rvargas wrote:
> On 2012/04/26 15:46:54, gavinp wrote:
> > Don't we mean "first" and "second" here?
> 
> I'm sorry I don't follow you here. What is first and second?. This is the
third
> and fourth values that this function receives from calling CheckListSection...
> so we have up to two nodes seen in each direction, but this code really
doesn't
> have a need (so far) to differentiate between them. We may need that in the
> future if we attempt to fix the list.
> 
> So that's what I have four generic names, because they are just four generic
> nodes.

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:812:
UMA_HISTOGRAM_CUSTOM_COUNTS("DiskCache.LostItems",
On 2012/04/27 00:18:10, rvargas wrote:
> On 2012/04/26 15:46:54, gavinp wrote:
> > So if I understand the comment, the range is  -9 to +8.   The name says it's
> the
> > number of lost items.  The call to HISTOGRAM_CUSTOM_COUNTS puts the minimum
as
> > 0, which is histogramspeak for 1, which is really 0 since all underflow will
> be
> > 0.  The maximum is given as 2000, and 75 buckets are allocated, even though
we
> > just said we only want 18.
> > 
> > UMA_HISTOGRAM_ENUMERATION("DiskCache.LostItemsPlus10", expected -
total_items
> +
> > 10, 18);  // comment justifying 18.
> 
> Nope. IMO, the histogram ranges (log ones) are kind of broken, and it boils
down
> to "try something until it looks like what you want". What I want is a log
scale
> with enough range to see if we miss 1000 items on a list, to see if we miss
the
> stop markers (even though we have two to reduce that chance), and get a
> reasonable resolution around 0.
> 
> The comment states that the range is linear from -9 to +8.. that means that
all
> buckets are storing just one value... -10 (aka 0) is the negative overflow,
and
> I don't really know what it means... probably that the counter is wrong, maybe
> that we really missed the markers. From 19 to 2000 we have a log scale... and
we
> need 75 buckets in order to have that many linear buckets close to 0.

Aha!  This is right on the cusp (so it's probably not worth it), but in a
situation like this a custom range can be great; we could reduce the 55 buckets
you have used to extend the range up to 2000 to something like 20; saving some
ram in each client that's reporting this histogram.  But to save 55 buckets, the
code would have to be awfully short.

> 
> From the point of view of seeing results, it's quite easy: just subtract 10
from
> the bucket that you're looking at.
> 
> Note that the actual histogram description is where the "user" should know
> what's being measured.

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:825: int error = list * 10;
Yeah, I thought as much.  It's a shame we can't put that in the name, at least
of the histogram at least... It's not just an error, it's the type of the list
too.  ListErrorWithType ?

On 2012/04/27 00:18:10, rvargas wrote:
> On 2012/04/26 15:46:54, gavinp wrote:
> > 10?  And not 9?
> 
> I'm going for something that is easier to read (when looking at the
histogram),
> so decimal system works better. This is just a standard "packed" value, in
> decimal.

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:871: CacheAddr next = (dir == FORWARD) ?
node->Data()->next : node->Data()->prev;
OK, so what motivates me on this is that I want callers to be obvious and easy
to read, and I want the executing code to be easy to read.  Adding an automatic
with a simple definition susceptible to the optimizer can really help with
that..

But you're right, this is not getting ridiculous, it is ridiculous we can go
back to calling the bool "is_forward" or something if both callers do it, since
it is private.  But these (dir == FORWARD) have got to go!

On 2012/04/27 00:18:10, rvargas wrote:
> This is getting ridiculous. The natural value is bool, not an enum... if you
> really want an enum, then live with it. I strongly believe that the previous
> version is better than this one, and having even more noise around to try and
> fix the issues introduced by that decision is not "the right thing to do".
> 
> On 2012/04/26 15:46:54, gavinp wrote:
> > This is really pessimal, including dir == FORWARD everywhere.  I realise it
> was
> > a bool, and I pushed hard for the parameter to be a bool, but why not have
it
> > both ways?
> > 
> > const bool is_forward = (dir == FORWARD); 
> > 
> > hen we have an interface that makes callers readable, and usage is readable.
>

[rvargas (doing something else), 2012-04-30 22:03:00]

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc
File net/disk_cache/rankings.cc (right):

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:789: Addr last3, last4;
On 2012/04/30 18:57:15, gavinp wrote:
> If I'm moving backwards, then when I reach the end, isn't that the beginning?

sort of... but that would confuse me. The returned values would be the closest
nodes to the head that were seen by that function, but that doesn't really make
them the beginning (or first nodes). I'm happy to rename them but I have yet not
found what would they be.

Consider that the list may be completely broken, not just by a misplaced node,
but by whole pages not reaching disk... so there's no relationship between
last1/2 and last3/4, as in there's no way to tell which nodes are really closer
to the head or tail... they may reflect just two views (in time) of the list,
and adding too much meaning to what they are (other than the "last node seen by
a given search") may be misleading.

> 
> On 2012/04/27 00:18:10, rvargas wrote:
> > On 2012/04/26 15:46:54, gavinp wrote:
> > > Don't we mean "first" and "second" here?
> > 
> > I'm sorry I don't follow you here. What is first and second?. This is the
> third
> > and fourth values that this function receives from calling
CheckListSection...
> > so we have up to two nodes seen in each direction, but this code really
> doesn't
> > have a need (so far) to differentiate between them. We may need that in the
> > future if we attempt to fix the list.
> > 
> > So that's what I have four generic names, because they are just four generic
> > nodes.
>

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:812:
UMA_HISTOGRAM_CUSTOM_COUNTS("DiskCache.LostItems",
Note that I'm actually happy with the usual 50 buckets that we have (in terms of
enough resolution and uniformity). In this case, I was also looking for that,
except with a custom range that went somewhat into the negative numbers...
ideally, [-x, x] should generate log shaped scales into the positive and
negative numbers... but it doesn't. I discussed this briefly with jar while
writing this code, and he didn't seem too inclined to actually fix the default
behavior (and in any case that was not going to happen before landing this CL).

So going to 75 buckets and a single histogram seemed better than 100 buckets
split into two histograms... especially because I don't really think we need the
same resolution in the negative side (I would have used something like [-20,
2000] if that worked).

On 2012/04/30 18:57:15, gavinp wrote:
> On 2012/04/27 00:18:10, rvargas wrote:
> > On 2012/04/26 15:46:54, gavinp wrote:
> > > So if I understand the comment, the range is  -9 to +8.   The name says
it's
> > the
> > > number of lost items.  The call to HISTOGRAM_CUSTOM_COUNTS puts the
minimum
> as
> > > 0, which is histogramspeak for 1, which is really 0 since all underflow
will
> > be
> > > 0.  The maximum is given as 2000, and 75 buckets are allocated, even
though
> we
> > > just said we only want 18.
> > > 
> > > UMA_HISTOGRAM_ENUMERATION("DiskCache.LostItemsPlus10", expected -
> total_items
> > +
> > > 10, 18);  // comment justifying 18.
> > 
> > Nope. IMO, the histogram ranges (log ones) are kind of broken, and it boils
> down
> > to "try something until it looks like what you want". What I want is a log
> scale
> > with enough range to see if we miss 1000 items on a list, to see if we miss
> the
> > stop markers (even though we have two to reduce that chance), and get a
> > reasonable resolution around 0.
> > 
> > The comment states that the range is linear from -9 to +8.. that means that
> all
> > buckets are storing just one value... -10 (aka 0) is the negative overflow,
> and
> > I don't really know what it means... probably that the counter is wrong,
maybe
> > that we really missed the markers. From 19 to 2000 we have a log scale...
and
> we
> > need 75 buckets in order to have that many linear buckets close to 0.
> 
> Aha!  This is right on the cusp (so it's probably not worth it), but in a
> situation like this a custom range can be great; we could reduce the 55
buckets
> you have used to extend the range up to 2000 to something like 20; saving some
> ram in each client that's reporting this histogram.  But to save 55 buckets,
the
> code would have to be awfully short.
> 
> > 
> > From the point of view of seeing results, it's quite easy: just subtract 10
> from
> > the bucket that you're looking at.
> > 
> > Note that the actual histogram description is where the "user" should know
> > what's being measured.
>

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:825: int error = list * 10;
I was thinking over lunch.. sure, ...ErrorWithList (it's not really a type of
error, it is the list in which the error occurred)... but then that would make
it DiskCache.ListErrorWithList, and that looks pretty bad. (..AndList looks
equally bad to me)

On 2012/04/30 18:57:15, gavinp wrote:
> Yeah, I thought as much.  It's a shame we can't put that in the name, at least
> of the histogram at least... It's not just an error, it's the type of the list
> too.  ListErrorWithType ?
> 
> On 2012/04/27 00:18:10, rvargas wrote:
> > On 2012/04/26 15:46:54, gavinp wrote:
> > > 10?  And not 9?
> > 
> > I'm going for something that is easier to read (when looking at the
> histogram),
> > so decimal system works better. This is just a standard "packed" value, in
> > decimal.
>

http://codereview.chromium.org/10148001/diff/16001/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:871: CacheAddr next = (dir == FORWARD) ?
node->Data()->next : node->Data()->prev;
as you wish.

On 2012/04/30 18:57:15, gavinp wrote:
> OK, so what motivates me on this is that I want callers to be obvious and easy
> to read, and I want the executing code to be easy to read.  Adding an
automatic
> with a simple definition susceptible to the optimizer can really help with
> that..
> 
> But you're right, this is not getting ridiculous, it is ridiculous we can go
> back to calling the bool "is_forward" or something if both callers do it,
since
> it is private.  But these (dir == FORWARD) have got to go!
> 
> On 2012/04/27 00:18:10, rvargas wrote:
> > This is getting ridiculous. The natural value is bool, not an enum... if you
> > really want an enum, then live with it. I strongly believe that the previous
> > version is better than this one, and having even more noise around to try
and
> > fix the issues introduced by that decision is not "the right thing to do".
> > 
> > On 2012/04/26 15:46:54, gavinp wrote:
> > > This is really pessimal, including dir == FORWARD everywhere.  I realise
it
> > was
> > > a bool, and I pushed hard for the parameter to be a bool, but why not have
> it
> > > both ways?
> > > 
> > > const bool is_forward = (dir == FORWARD); 
> > > 
> > > hen we have an interface that makes callers readable, and usage is
readable.
> > 
>

http://codereview.chromium.org/10148001/diff/34002/net/disk_cache/rankings.cc
File net/disk_cache/rankings.cc (right):

http://codereview.chromium.org/10148001/diff/34002/net/disk_cache/rankings.cc...
net/disk_cache/rankings.cc:784: int rv = CheckListSection(list, last1, last2,
true, // Head to tail.
How about this then.

No name to mess, no changing the polarity of the variable, and 100% explicit to
the reader.

[gavinp, 2012-04-30 22:13:17]

LGTM.

Thanks for bearing with me as I read through this!

[rvargas (doing something else), 2012-04-30 23:31:05]

Thanks.

I uploaded what should be the final version (#5). (and will commit it when the
bots pass)

Sorry for the drag.

[commit-bot: I haz the power, 2012-05-01 03:12:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rvargas@chromium.org/10148001/44002

[commit-bot: I haz the power, 2012-05-01 04:58:39]

Change committed as 134678