Add histograms to track localStorage size and load time by size.

Add histograms to track localStorage size and load time by size.

I'm concerned that the original histograms are skewed by sample bias,
since most localStorage DBs are probably small.

Also introduce browser and renderer specific histograms. Kept the original histogram for the renderer one in order to maintain histogram data continuity.

BUG=none


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

[willchan no longer on Chromium, 2013-02-10 05:53:55]

Michael to review, added Jim in case he has advice on how to better use
histograms, and also a heads up for the following histograms.xml change.

Ideally it'd be nice to get this in on Monday.

[jar (doing other things), 2013-02-11 19:27:51]

I'm not an owner in this area... and my LGTM with nits may be of little help.

see commentary below.

https://codereview.chromium.org/12209085/diff/1/webkit/dom_storage/dom_storag...
File webkit/dom_storage/dom_storage_cached_area.cc (right):

https://codereview.chromium.org/12209085/diff/1/webkit/dom_storage/dom_storag...
webkit/dom_storage/dom_storage_cached_area.cc:177:
UMA_HISTOGRAM_TIMES("LocalStorage.TimeToPrimeLocalStorageUnder5MB",
Based on the above logic, the histograms are really:
Storage <100KB
vs
100KB < storage < 1MB
vs
1MB < storage < 5MB

As a result, the names of teh histogram are a tad deceptive.

You might either change the names, or adjust the logic to not have "else"
clauses, and just use "if" statements.

YMMV.

[michaeln, 2013-02-12 00:38:15]

https://codereview.chromium.org/12209085/diff/1/webkit/dom_storage/dom_storag...
File webkit/dom_storage/dom_storage_cached_area.cc (right):

https://codereview.chromium.org/12209085/diff/1/webkit/dom_storage/dom_storag...
webkit/dom_storage/dom_storage_cached_area.cc:166: // above what we see in
practice, since histograms can't change.
lgtm modulo the paranoia about 'too large'

... but it might not be measuring exactly what you want. This will measure the
time to prime the renderer's cache w/o regard for the state of the browser's
cache. From when we spoke, sounded like you were interested in measuring the
time to prime the browser's cache.

Maybe we'll see two spikes in the distribution, a shorter time for when the
browsers cache is already primed, and a longer time for when its not.

https://codereview.chromium.org/12209085/diff/1/webkit/dom_storage/dom_storag...
webkit/dom_storage/dom_storage_cached_area.cc:169: 0, 6 * 1024, 50);
What happens if we give this macro a value that's 'too large'? If the answer is
it crashes, probably worth a call to std::min.

[willchan no longer on Chromium, 2013-02-12 00:42:22]

https://codereview.chromium.org/12209085/diff/1/webkit/dom_storage/dom_storag...
File webkit/dom_storage/dom_storage_cached_area.cc (right):

https://codereview.chromium.org/12209085/diff/1/webkit/dom_storage/dom_storag...
webkit/dom_storage/dom_storage_cached_area.cc:166: // above what we see in
practice, since histograms can't change.
On 2013/02/12 00:38:15, michaeln wrote:
> lgtm modulo the paranoia about 'too large'
> 
> ... but it might not be measuring exactly what you want. This will measure the
> time to prime the renderer's cache w/o regard for the state of the browser's
> cache. From when we spoke, sounded like you were interested in measuring the
> time to prime the browser's cache.
> 
> Maybe we'll see two spikes in the distribution, a shorter time for when the
> browsers cache is already primed, and a longer time for when its not.

Is there a way to instrument this in a way so I can isolate out the browser and
renderer samples?

https://codereview.chromium.org/12209085/diff/1/webkit/dom_storage/dom_storag...
webkit/dom_storage/dom_storage_cached_area.cc:169: 0, 6 * 1024, 50);
On 2013/02/12 00:38:15, michaeln wrote:
> What happens if we give this macro a value that's 'too large'? If the answer
is
> it crashes, probably worth a call to std::min.

It doesn't crash, we just lose granularity. What's the maximum value returned by
MemoryBytesUsedByCache()? I wasn't sure how exact it was.

https://codereview.chromium.org/12209085/diff/1/webkit/dom_storage/dom_storag...
webkit/dom_storage/dom_storage_cached_area.cc:177:
UMA_HISTOGRAM_TIMES("LocalStorage.TimeToPrimeLocalStorageUnder5MB",
On 2013/02/11 19:27:51, jar wrote:
> Based on the above logic, the histograms are really:
> Storage <100KB
> vs
> 100KB < storage < 1MB
> vs
> 1MB < storage < 5MB
> 
> As a result, the names of teh histogram are a tad deceptive.
> 
> You might either change the names, or adjust the logic to not have "else"
> clauses, and just use "if" statements.
> 
> YMMV.

Good point. I'll rename once I settle the above stuff with Michael.

[michaeln, 2013-02-12 01:17:12]

https://codereview.chromium.org/12209085/diff/1/webkit/dom_storage/dom_storag...
File webkit/dom_storage/dom_storage_cached_area.cc (right):

https://codereview.chromium.org/12209085/diff/1/webkit/dom_storage/dom_storag...
webkit/dom_storage/dom_storage_cached_area.cc:166: // above what we see in
practice, since histograms can't change.
> Is there a way to instrument this in a way so I can isolate out the browser
and
> renderer samples?

Not from this class|method, we can't distinguish here what the state of the
browser is.

See DomStorageArea::InitialImportIfNeeded() for where the browser's cache (a
map) is loaded out of the db.

https://codereview.chromium.org/12209085/diff/1/webkit/dom_storage/dom_storag...
webkit/dom_storage/dom_storage_cached_area.cc:169: 0, 6 * 1024, 50);
> It doesn't crash

sgtm!

> MemoryBytesUsedByCache()? 

There is some room for an origin to go over budget. I didn't remember by what
amount exactly so I dug around in the code.

// In the browser process we allow some overage to
// accommodate concurrent writes from different renderers
// that were allowed because the limit imposed in the renderer
// wasn't exceeded.
const size_t kPerAreaOverQuotaAllowance = 100 * 1024;

[willchan no longer on Chromium, 2013-02-12 01:48:41]

OK updated with browser histograms and bucket renaming.

[michaeln, 2013-02-12 01:49:55]

lgtm!

https://codereview.chromium.org/12209085/diff/1003/webkit/dom_storage/dom_sto...
File webkit/dom_storage/dom_storage_cached_area.cc (right):

https://codereview.chromium.org/12209085/diff/1003/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_cached_area.cc:165: size_t local_storage_size_kb
= MemoryBytesUsedByCache() / 1024;
could use map_->bytes_used() here

[willchan no longer on Chromium, 2013-02-12 01:53:41]

done, thanks for the very quick reviews. sending to CQ.

https://codereview.chromium.org/12209085/diff/1003/webkit/dom_storage/dom_sto...
File webkit/dom_storage/dom_storage_cached_area.cc (right):

https://codereview.chromium.org/12209085/diff/1003/webkit/dom_storage/dom_sto...
webkit/dom_storage/dom_storage_cached_area.cc:165: size_t local_storage_size_kb
= MemoryBytesUsedByCache() / 1024;
On 2013/02/12 01:49:55, michaeln wrote:
> could use map_->bytes_used() here

Done.

[commit-bot: I haz the power, 2013-02-12 01:55:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/willchan@chromium.org/12209085/1005

[commit-bot: I haz the power, 2013-02-12 04:56:13]

Change committed as 181855