[sql] Cleanup open and close error histograms.

[sql] Cleanup open and close error histograms.

Cleanup open-time histograms to use the sparse framework so that
extended histograms can be recorded - also add entries to the xml
file.

Add a histogram to track close-time failures.  This should never fail,
but if it is it probably leaks memory and file handles, and possibly
locks.

BUG=none

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

[Scott Hess - ex-Googler, 2013-07-15 17:40:03]

Bit of a loose-ends cleanup.

[erikwright (departed), 2013-07-17 17:08:56]

I've never heard of SPARSE_SLOWLY and it seems to be undocumented... If you know
what it does could you add documentation to sparse_histogram.h? Or else ask jar@
or someone to do it?

[Scott Hess - ex-Googler, 2013-07-17 17:21:25]

On 2013/07/17 17:08:56, erikwright wrote:
> I've never heard of SPARSE_SLOWLY and it seems to be undocumented... If you
know
> what it does could you add documentation to sparse_histogram.h? Or else ask
jar@
> or someone to do it?

Looks like the author of most of it is gone, so I'll ping jar (he'll need to
OWNERS-stamp the histograms.xml change anyhow).

My understanding is that a regular histogram is a vector of contiguous buckets,
whereas a sparse histogram is a collection of bucket structures.  The downside
is that the sparse version requires more memory.  So a regular histogram is
better for a densely-populated series, or a series which can be kept relatively
compact, whereas a sparse histogram is better for a large or sparse series. 
Apparently error codes are the canonical good thing for sparse histograms.

[jar (doing other things), 2013-07-17 23:52:22]

Shess' description is on target.

Sparse histograms use a stl::map<>, which means they are "slower," and most
critically must be locked to preclude corruption while updating (adding a
sample).  It also means that adding a sample may induce a call to malloc,
further slowing the sampling process.

Ordinary histograms use an stl::vector<>, which is preallocated, and can
(with a bit of racing, that won't hurt anyone) be updated without a lock,
super fast.   ...but if you want to use 'em, you need to know exactly how
many buckets to pre-allocate in the vector.  That can suck up a lot of
memory if you wanted a ton of buckets, especially if most of those buckets
are NEVER used :-(.

With error codes, most users *never* send any, but when they do, they often
have a large selection to choose from, and only send one or two.  Using a
map to lazily allocate the one or two buckets  (entries in the stl::map
from a sample, to a count) works very nicely.  ...and errors are so rare,
that we don't mind the time it takes to tally them in an stl::map, under a
lock, calling malloc ;-).




On Wed, Jul 17, 2013 at 10:21 AM, <shess@chromium.org> wrote:

> On 2013/07/17 17:08:56, erikwright wrote:
>
>> I've never heard of SPARSE_SLOWLY and it seems to be undocumented... If
>> you
>>
> know
>
>> what it does could you add documentation to sparse_histogram.h? Or else
>> ask
>>
> jar@
>
>> or someone to do it?
>>
>
> Looks like the author of most of it is gone, so I'll ping jar (he'll need
> to
> OWNERS-stamp the histograms.xml change anyhow).
>
> My understanding is that a regular histogram is a vector of contiguous
> buckets,
> whereas a sparse histogram is a collection of bucket structures.  The
> downside
> is that the sparse version requires more memory.  So a regular histogram is
> better for a densely-populated series, or a series which can be kept
> relatively
> compact, whereas a sparse histogram is better for a large or sparse series.
> Apparently error codes are the canonical good thing for sparse histograms.
>
>
https://codereview.chromium.**org/18978012/<https://codereview.chromium.org/1...
>

[Scott Hess - ex-Googler, 2013-07-22 17:57:39]

OK, we all good?  Pinging Erik for sql/, Jim for histograms.xml.

Jim, would you mind if I were to place some synthesis of my comment and yours at
the top of the sparse histogram header (in a separate CL)?  Or would you rather
have someone who actually knows the implementation handle it?

https://codereview.chromium.org/18978012/diff/3001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/18978012/diff/3001/sql/connection.cc#newcode239
sql/connection.cc:239: // TODO(shess): Histogram for failure.
Erp, removing this TODO.

[erikwright (departed), 2013-07-22 18:15:36]

sql/ LGTM.

[Scott Hess - ex-Googler, 2013-07-23 19:12:41]

Jim, did histograms.xml change look good?  Thanks.

[jar (doing other things), 2013-07-24 02:43:55]

histograms.xml LGTM

If you want to add comments regarding use of sparse histograms to the histogram
source files, that would be great... but should probably go into a different CL.
 Thanks!

[Scott Hess - ex-Googler, 2013-07-24 03:14:37]

On 2013/07/24 02:43:55, jar wrote:
> If you want to add comments regarding use of sparse histograms to the
histogram
> source files, that would be great... but should probably go into a different
CL.

I agree.

[commit-bot: I haz the power, 2013-07-24 03:16:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/18978012/18001

[commit-bot: I haz the power, 2013-07-24 05:04:31]

Change committed as 213345