ServiceWorker: Add UMA to measure execution times of oninstall/onfetch

ServiceWorker: Add UMA to measure execution times of oninstall/onfetch

BUG=407997
TEST=N/A

Committed: https://crrev.com/6b6ed6be7fb3516edec7b044d58aa3b718ca5bec
Cr-Commit-Position: refs/heads/master@{#297352}

[shimazu, 2014-09-26 04:37:11]

shimazu@chromium.org changed reviewers:
+ mpearson@chromium.org, nhiroki@chromium.org

[shimazu, 2014-09-26 04:37:12]

mpearson@chromium.org: Please review changes in histograms.xml.

nhiroki@chromium.org: Please review changes in the others
(content/browser/service_worker/*).

[nhiroki, 2014-09-26 07:28:18]

Thank you for working on this!

As we chatted offline, measurement spans seem too wide and we could measure more
accurate execution time of each JS event.

https://codereview.chromium.org/604193002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.h (right):

https://codereview.chromium.org/604193002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.h:170:
IDMap<base::TimeTicks, IDMapExternalPointer> registration_start_timings_;
Why don't you use std::map?

https://codereview.chromium.org/604193002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_metrics.cc (right):

https://codereview.chromium.org/604193002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_metrics.cc:60:
ServiceWorkerMetrics::MeasureFetchEventExecutionTime(base::TimeDelta delta) {
nit: Wrapping at the argument would be more common in our codebase.

Can you use const-ref for the argument?

https://codereview.chromium.org/604193002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_metrics.cc:66:
ServiceWorkerMetrics::MeasureRegistrationTime(base::TimeDelta delta) {
ditto.

[Mark P, 2014-09-26 16:20:48]

https://codereview.chromium.org/604193002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/604193002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:29347: +    Execution time of onfetch
event. This is mainly composed by a execution of
For my information, are these recorded regardless of how the event terminates
(e.g., success or throws error)?  If so, no need to make any revisions.

[shimazu, 2014-09-29 08:15:47]

Move the place of measurement, and change the descriptions in histograms.xml.

Could you review again? :)

https://codereview.chromium.org/604193002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.h (right):

https://codereview.chromium.org/604193002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.h:170:
IDMap<base::TimeTicks, IDMapExternalPointer> registration_start_timings_;
On 2014/09/26 07:28:17, nhiroki wrote:
> Why don't you use std::map?

Done.

https://codereview.chromium.org/604193002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/604193002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:29347: +    Execution time of onfetch
event. This is mainly composed by a execution of
On 2014/09/26 16:20:48, Mark P wrote:
> For my information, are these recorded regardless of how the event terminates
> (e.g., success or throws error)?  If so, no need to make any revisions.

Done.

[nhiroki, 2014-09-29 09:41:23]

lgtm with a nit

https://codereview.chromium.org/604193002/diff/40001/content/renderer/service...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/604193002/diff/40001/content/renderer/service...
content/renderer/service_worker/service_worker_script_context.cc:7: #include
<map>
nit: can you remove this include?

[Mark P, 2014-09-29 18:00:36]

lgtm


assuming the answer below is yes, lgtm

https://codereview.chromium.org/604193002/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/604193002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:29425: +  <summary>Execution time of
ServiceWorkerGlobalScope.onactivate.</summary>
On 2014/09/29 08:15:47, makoto wrote:
> On 2014/09/26 16:20:48, Mark P wrote:
> > For my information, are these recorded regardless of how the event
terminates
> > (e.g., success or throws error)?  If so, no need to make any revisions.
> 
> Done.

Does "Done" mean "Yes"?

[shimazu, 2014-09-30 01:31:09]

Thanks for your reviews.
I'll commit it.

https://codereview.chromium.org/604193002/diff/40001/content/renderer/service...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/604193002/diff/40001/content/renderer/service...
content/renderer/service_worker/service_worker_script_context.cc:7: #include
<map>
On 2014/09/29 09:41:23, nhiroki wrote:
> nit: can you remove this include?

Done.

https://codereview.chromium.org/604193002/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/604193002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:29425: +  <summary>Execution time of
ServiceWorkerGlobalScope.onactivate.</summary>
On 2014/09/29 18:00:36, Mark P wrote:
> On 2014/09/29 08:15:47, makoto wrote:
> > On 2014/09/26 16:20:48, Mark P wrote:
> > > For my information, are these recorded regardless of how the event
> terminates
> > > (e.g., success or throws error)?  If so, no need to make any revisions.
> > 
> > Done.
> 
> Does "Done" mean "Yes"?

Oops, sorry. Yes.

[shimazu, 2014-09-30 01:31:21]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2014-09-30 01:32:07]

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

[commit-bot: I haz the power, 2014-09-30 03:31:28]

Committed patchset #4 (id:60001) as bb4479172385962f7c44c302fb2359ca3107a861

[commit-bot: I haz the power, 2014-09-30 03:32:15]

Patchset 4 (id:??) landed as
https://crrev.com/6b6ed6be7fb3516edec7b044d58aa3b718ca5bec
Cr-Commit-Position: refs/heads/master@{#297352}