Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(394)

Issue 812843005: [Storage] Added histograms for blob storage (Closed)

Created:
5 years, 11 months ago by dmurph
Modified:
5 years, 11 months ago
CC:
asvitkine+watch_chromium.org, chromium-reviews, jsbell
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Storage] Added histograms for blob storage For finding: * Size distribution of blob storage * Size distribution of blob items * Number of cases when we are exceeding internal memory for blobs. See here for info: https://bit.ly/AutoBlobToDisk BUG=375297 Committed: https://crrev.com/69e28f6f37f80b9a472a41ba8198d5818508100f Cr-Commit-Position: refs/heads/master@{#310367}

Patch Set 1 #

Total comments: 7

Patch Set 2 : Nits and histogram change #

Patch Set 3 : fixed GT check with unsigned 0 #

Patch Set 4 : clang format & windows compile issue #

Patch Set 5 : cleaner windows compile fix #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+60 lines, -12 lines) Patch
M storage/browser/blob/blob_storage_context.cc View 1 2 3 4 4 chunks +19 lines, -12 lines 1 comment Download
M tools/metrics/histograms/histograms.xml View 2 chunks +41 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (4 generated)
dmurph
Hello jar & michaeln, Please take a look at these histograms. These are being added ...
5 years, 11 months ago (2015-01-05 23:04:28 UTC) #2
jar (doing other things)
+asvitkine for histograms.xml
5 years, 11 months ago (2015-01-06 00:22:45 UTC) #4
Alexei Svitkine (slow)
Generally looks good - just a couple comments/questions I was wondering about. https://codereview.chromium.org/812843005/diff/1/storage/browser/blob/blob_storage_context.cc File storage/browser/blob/blob_storage_context.cc ...
5 years, 11 months ago (2015-01-06 16:47:42 UTC) #5
dmurph
https://codereview.chromium.org/812843005/diff/1/storage/browser/blob/blob_storage_context.cc File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/812843005/diff/1/storage/browser/blob/blob_storage_context.cc#newcode148 storage/browser/blob/blob_storage_context.cc:148: DCHECK(length > 0); On 2015/01/06 16:47:42, Alexei Svitkine wrote: ...
5 years, 11 months ago (2015-01-06 20:15:44 UTC) #6
Alexei Svitkine (slow)
LGTM https://codereview.chromium.org/812843005/diff/1/storage/browser/blob/blob_storage_context.cc File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/812843005/diff/1/storage/browser/blob/blob_storage_context.cc#newcode190 storage/browser/blob/blob_storage_context.cc:190: memory_usage_ / 1024); On 2015/01/06 20:15:44, dmurph wrote: ...
5 years, 11 months ago (2015-01-06 20:22:48 UTC) #7
dmurph
Hi Antoine, I have a simple UMA stats CL here that needs approval for storage/. ...
5 years, 11 months ago (2015-01-07 19:00:22 UTC) #9
piman
lgtm
5 years, 11 months ago (2015-01-07 19:27:36 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/812843005/80001
5 years, 11 months ago (2015-01-07 20:30:50 UTC) #12
commit-bot: I haz the power
Committed patchset #5 (id:80001)
5 years, 11 months ago (2015-01-07 21:08:31 UTC) #13
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/69e28f6f37f80b9a472a41ba8198d5818508100f Cr-Commit-Position: refs/heads/master@{#310367}
5 years, 11 months ago (2015-01-07 21:10:42 UTC) #14
michaeln
What does having a histogram of total usage before and also after the addition of ...
5 years, 11 months ago (2015-01-07 21:30:54 UTC) #15
dmurph
On 2015/01/07 at 21:30:54, michaeln wrote: > What does having a histogram of total usage ...
5 years, 11 months ago (2015-01-07 21:53:30 UTC) #16
michaeln
On 2015/01/07 21:53:30, dmurph wrote: > On 2015/01/07 at 21:30:54, michaeln wrote: > > What ...
5 years, 11 months ago (2015-01-07 22:02:34 UTC) #17
dmurph
On 2015/01/07 at 22:02:34, michaeln wrote: > On 2015/01/07 21:53:30, dmurph wrote: > > On ...
5 years, 11 months ago (2015-01-07 22:09:39 UTC) #18
dmurph
5 years, 11 months ago (2015-01-07 22:10:27 UTC) #19
Message was sent while issue was closed.
On 2015/01/07 at 22:09:39, dmurph wrote:
> On 2015/01/07 at 22:02:34, michaeln wrote:
> > On 2015/01/07 21:53:30, dmurph wrote:
> > > On 2015/01/07 at 21:30:54, michaeln wrote:
> > > > What does having a histogram of total usage before and also after the
addition
> > > of an individual item tell us? Won't they be very similar plots?
> > > > 
> > > >
> > >
https://codereview.chromium.org/812843005/diff/80001/storage/browser/blob/blo...
> > > > File storage/browser/blob/blob_storage_context.cc (right):
> > > > 
> > > >
> > >
https://codereview.chromium.org/812843005/diff/80001/storage/browser/blob/blo...
> > > > storage/browser/blob/blob_storage_context.cc:183:
> > > UMA_HISTOGRAM_COUNTS("Storage.Blob.StorageSizeAfterAppend",
> > > > Would it make sense to use either UMA_HISTOGRAM_MEMORY_MB or
> > > UMA_HISTOGRAM_MEMORY_KB here?
> > > 
> > > The macros amount to similar things.  As outlined above, I want to see the
> > > distribution of the final (or max) memory usage on clients for the blob
storage.
> > >  The only way to do this, as far as I can see (asvitkine agreed) is to
record
> > > both a 'before' and 'after' histogram, and then subtract the 'before' from
the
> > > 'after' manually to get the final distribution (or max memory usage up
till the
> > > last sample on the given machine).
> > > 
> > > Example for a machine:
> > > before: 0, 12, 50, 55
> > > after: 12, 50, 55, 102
> > > after - before: 102
> > 
> > i see, that will take some processing to produce
> > 
> > do you know if uma is good about capturing events during shutdown? if so,
you could track the high water mark and provide one sample when the context is
deleted.
> 
> I don't think that is guaranteed at all, as it is a super
> asynchronous. It usually takes a couple of reloads in the
> about://histograms for stats to show up. Spending 5 minutes
> to do some subtraction seems totally fine to me, as this
> isn't going to be a constantly-needed stat, and more 1-off
> windows into the world.

Also, this is pattern is already used.  See
“GPU.ProgramCache.MemorySizeBeforeKb” and
“GPU.ProgramCache.MemorySizeAfterKb”

Powered by Google App Engine
This is Rietveld 408576698