[ServiceWorker] Cache & CacheStorage operation histograms

[ServiceWorker] Cache & CacheStorage operation histograms

BUG=439529
Committed: https://crrev.com/1dd2bdd0f95e553481b988a83105e01dc7b2e261
Cr-Commit-Position: refs/heads/master@{#307995}

[dmurph, 2014-12-11 01:15:42]

dmurph@chromium.org changed reviewers:
+ horo@chromium.org, jsbell@chromium.org

[dmurph, 2014-12-11 01:15:43]

horo & jsbell,

Please take a look at this patch, it adds local histograms for the Cache
operations.  This looks sufficient for CacheStorage, but I'm wondering if we
should trace these deeper down for the batch operations.  Or perhaps have new
histograms for all the batch calls, like "ServiceWorkerCache.BatchPut", etc. 
That way we can measure them separately.

We can get the local histograms in first, then modify the histograms.xml and
change them to the "UMA_" variant.

[horo, 2014-12-11 06:43:22]

On 2014/12/11 01:15:43, dmurph wrote:
> horo & jsbell,
> 
> Please take a look at this patch, it adds local histograms for the Cache
> operations.  This looks sufficient for CacheStorage, but I'm wondering if we
> should trace these deeper down for the batch operations.  Or perhaps have new
> histograms for all the batch calls, like "ServiceWorkerCache.BatchPut", etc. 
> That way we can measure them separately.
> 
> We can get the local histograms in first, then modify the histograms.xml and
> change them to the "UMA_" variant.

looks good.
But how about measuring the time in the browser process?
If you measure in the browser process, you can get the time excluding the IPC
operation.

[dmurph, 2014-12-11 08:20:23]

On 2014/12/11 06:43:22, horo wrote:
> On 2014/12/11 01:15:43, dmurph wrote:
> > horo & jsbell,
> > 
> > Please take a look at this patch, it adds local histograms for the Cache
> > operations.  This looks sufficient for CacheStorage, but I'm wondering if we
> > should trace these deeper down for the batch operations.  Or perhaps have
new
> > histograms for all the batch calls, like "ServiceWorkerCache.BatchPut", etc.

> > That way we can measure them separately.
> > 
> > We can get the local histograms in first, then modify the histograms.xml and
> > change them to the "UMA_" variant.
> 
> looks good.
> But how about measuring the time in the browser process?
> If you measure in the browser process, you can get the time excluding the IPC
> operation.

Here was my reasoning: we do a couple operations here before the callback, like
transformations to WebRequests (and string operations, see top of the file and
around line 608).  These seem like they could have a performance hit, so I
thought it might be a good idea to include them in the stats.  If you still want
me to move these to the browser process, please confirm.  Thanks!

[horo, 2014-12-11 08:29:11]

On 2014/12/11 08:20:23, dmurph wrote:
> On 2014/12/11 06:43:22, horo wrote:
> > On 2014/12/11 01:15:43, dmurph wrote:
> > > horo & jsbell,
> > > 
> > > Please take a look at this patch, it adds local histograms for the Cache
> > > operations.  This looks sufficient for CacheStorage, but I'm wondering if
we
> > > should trace these deeper down for the batch operations.  Or perhaps have
> new
> > > histograms for all the batch calls, like "ServiceWorkerCache.BatchPut",
etc.
> 
> > > That way we can measure them separately.
> > > 
> > > We can get the local histograms in first, then modify the histograms.xml
and
> > > change them to the "UMA_" variant.
> > 
> > looks good.
> > But how about measuring the time in the browser process?
> > If you measure in the browser process, you can get the time excluding the
IPC
> > operation.
> 
> Here was my reasoning: we do a couple operations here before the callback,
like
> transformations to WebRequests (and string operations, see top of the file and
> around line 608).  These seem like they could have a performance hit, so I
> thought it might be a good idea to include them in the stats.  If you still
want
> me to move these to the browser process, please confirm.  Thanks!

OK. I don't have strong opinions.
lgtm.

[dmurph, 2014-12-11 20:11:01]

The CQ bit was checked by dmurph@chromium.org

[commit-bot: I haz the power, 2014-12-11 20:12:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/797463002/1

[commit-bot: I haz the power, 2014-12-11 23:12:48]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-12-11 23:14:48]

Patchset 1 (id:??) landed as
https://crrev.com/1dd2bdd0f95e553481b988a83105e01dc7b2e261
Cr-Commit-Position: refs/heads/master@{#307995}