|
|
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
Messages
Total messages: 19 (4 generated)
dmurph@chromium.org changed reviewers: + jar@chromium.org, michaeln@chromium.org
Hello jar & michaeln, Please take a look at these histograms. These are being added per discussion here: https://bit.ly/AutoBlobToDisk to determine what blob usage is like. Daniel
jar@chromium.org changed reviewers: + asvitkine@chromium.org - jar@chromium.org
+asvitkine for histograms.xml
Generally looks good - just a couple comments/questions I was wondering about. https://codereview.chromium.org/812843005/diff/1/storage/browser/blob/blob_st... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/812843005/diff/1/storage/browser/blob/blob_st... storage/browser/blob/blob_storage_context.cc:148: DCHECK(length > 0); Nit: DCHECK_GT https://codereview.chromium.org/812843005/diff/1/storage/browser/blob/blob_st... storage/browser/blob/blob_storage_context.cc:190: memory_usage_ / 1024); Just wondering why you actually need both the Before and After versions of this histogram. I suspect their shapes would look rather similar - what's the purpose of needing both? https://codereview.chromium.org/812843005/diff/1/storage/browser/blob/blob_st... storage/browser/blob/blob_storage_context.cc:195: UMA_HISTOGRAM_BOOLEAN("Storage.BlobItem.ExceededMemory", exceeded_memory); Since exceeded memory can only happen on TYPE_BLOB, would it make more sense to log this in that block? That way, you get the ratio of TYPE_BLOB's that exceeded memory rather than of everything... I guess you can derive one from the other with the help of the other metrics, but if you're actually interested in ratio of blobs that exceeded, would be better to be in the case.
https://codereview.chromium.org/812843005/diff/1/storage/browser/blob/blob_st... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/812843005/diff/1/storage/browser/blob/blob_st... storage/browser/blob/blob_storage_context.cc:148: DCHECK(length > 0); On 2015/01/06 16:47:42, Alexei Svitkine wrote: > Nit: DCHECK_GT Done. https://codereview.chromium.org/812843005/diff/1/storage/browser/blob/blob_st... storage/browser/blob/blob_storage_context.cc:190: memory_usage_ / 1024); On 2015/01/06 16:47:41, Alexei Svitkine wrote: > Just wondering why you actually need both the Before and After versions of this > histogram. > > I suspect their shapes would look rather similar - what's the purpose of needing > both? I want to find the distribution of the final storage size (or maximum storage size) in all machines. I can do this by logging all of the 'befores' and 'afters', and then subtracting all of the befores from the afters, which gives me the final 'after', which is the maximum (or final) storage size on that machine. If you know a better way to do this I would definitely be interested. https://codereview.chromium.org/812843005/diff/1/storage/browser/blob/blob_st... storage/browser/blob/blob_storage_context.cc:195: UMA_HISTOGRAM_BOOLEAN("Storage.BlobItem.ExceededMemory", exceeded_memory); On 2015/01/06 16:47:42, Alexei Svitkine wrote: > Since exceeded memory can only happen on TYPE_BLOB, would it make more sense to > log this in that block? That way, you get the ratio of TYPE_BLOB's that exceeded > memory rather than of everything... > > I guess you can derive one from the other with the help of the other metrics, > but if you're actually interested in ratio of blobs that exceeded, would be > better to be in the case. It's set in both TYPE_BLOB and TYPE_BYTES However, I believe this isn't the best stat for us, as we want memory exceeded ratio for blobs, not blob items. Changed.
LGTM https://codereview.chromium.org/812843005/diff/1/storage/browser/blob/blob_st... File storage/browser/blob/blob_storage_context.cc (right): https://codereview.chromium.org/812843005/diff/1/storage/browser/blob/blob_st... storage/browser/blob/blob_storage_context.cc:190: memory_usage_ / 1024); On 2015/01/06 20:15:44, dmurph wrote: > On 2015/01/06 16:47:41, Alexei Svitkine wrote: > > Just wondering why you actually need both the Before and After versions of > this > > histogram. > > > > I suspect their shapes would look rather similar - what's the purpose of > needing > > both? > > I want to find the distribution of the final storage size (or maximum storage > size) in all machines. I can do this by logging all of the 'befores' and > 'afters', and then subtracting all of the befores from the afters, which gives > me the final 'after', which is the maximum (or final) storage size on that > machine. > > If you know a better way to do this I would definitely be interested. Ah, I see. You'll have to deal with some skew (i.e. when the before was logged on a previous day than the after and when the next after will be on the next day, etc), but I don't have a better suggestion for this, unfortunately - so this is OK.
dmurph@chromium.org changed reviewers: + piman@chromium.org
Hi Antoine, I have a simple UMA stats CL here that needs approval for storage/. Do you have a second to look? Thanks, Daniel
lgtm
The CQ bit was checked by dmurph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/812843005/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/69e28f6f37f80b9a472a41ba8198d5818508100f Cr-Commit-Position: refs/heads/master@{#310367}
Message was sent while issue was closed.
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?
Message was sent while issue was closed.
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
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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.
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” |