service worker: Add UMA for start worker IPC latency. [chromium/src : master]
0 views
Skip to first unread message
Matt Falkenhagen (Gerrit)
Jun 22, 2017, 3:46:28 AM6/22/17
to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Darin Fisher, Taiju Tsuiki, chromium...@chromium.org, Hiroki Nakagawa, John Abd-El-Malek, Michael Nordman, Adam Barth, Aaron Boodman
Matt Falkenhagen uploaded patch set #2 to this change.
service worker: Add UMA for start worker IPC latency.
The motivation is to get a bound on how much optimization can be done before
the start worker message is received. This includes work to startup a renderer
faster and off-main-thread start worker.
In order to try to distinguish between time spent in process creation/setup vs
main thread contention, I tried to estimate when the renderer is launched
and ready to receive messages via RenderThreadImpl::Init(). However
it looks like on browser startup, the renderer setup finishes quickly
(before start worker is sent) but there a huge message latency (500 ms).
Let me know if you have any better ideas.
The CL also sets the path for sending timing information from the renderer to
browser, so we can record the timing directly in the renderer and remove IPCs
we currently use to keep track of the start sequence on the browser-side.
Because these IPCs are affected by IO thread contention on the browser side,
they can be causing inaccurate timing information logged to UMA.
Bug: 561209
Change-Id: I4d000ba2a66c6a0576bb6c0eee21b35590db12d4
---
M content/browser/service_worker/embedded_worker_instance.cc
M content/browser/service_worker/embedded_worker_instance.h
M content/browser/service_worker/embedded_worker_test_helper.cc
M content/browser/service_worker/service_worker_metrics.cc
M content/browser/service_worker/service_worker_metrics.h
M content/browser/service_worker/service_worker_metrics_unittest.cc
M content/common/service_worker/embedded_worker.mojom
M content/common/service_worker/embedded_worker_messages.h
M content/common/service_worker/embedded_worker_start_params.h
M content/renderer/render_thread_impl.cc
M content/renderer/service_worker/embedded_worker_instance_client_impl.cc
M content/renderer/service_worker/embedded_worker_instance_client_impl.h
M content/renderer/service_worker/service_worker_context_client.cc
M content/renderer/service_worker/service_worker_context_client.h
M third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h
M tools/metrics/histograms/enums.xml
M tools/metrics/histograms/histograms.xml
17 files changed, 246 insertions(+), 37 deletions(-)
To view, visit change 544718. To unsubscribe, visit settings.
Matt Falkenhagen (Gerrit)
Jun 22, 2017, 3:54:36 AM6/22/17
to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Darin Fisher, Taiju Tsuiki, chromium...@chromium.org, Hiroki Nakagawa, John Abd-El-Malek, Michael Nordman, Adam Barth, Aaron Boodman
Matt Falkenhagen uploaded patch set #3 to this change.
service worker: Add UMA for start worker IPC latency.
The motivation is to get a bound on how much optimization can be done before
the start worker message is received. This includes work to startup a renderer
faster and off-main-thread start worker.
In order to try to distinguish between time spent in process creation/setup vs
main thread contention, I tried to estimate when the renderer is launched
and ready to receive messages via RenderThreadImpl::Init(). However
it looks like on browser startup, the renderer setup finishes quickly
(before start worker is sent) but there a huge message latency (500 ms).
I suspect this can never be super accurate anyway because the renderer
may have been initialized long ago, was suspended, and we need to wait
for it to resume.
Matt Falkenhagen (Gerrit)
Jun 22, 2017, 4:04:08 AM6/22/17
to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Makoto Shimazu, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
Matt Falkenhagen posted comments on this change.
Patch set 4:Commit-Queue +1
shimazu: could you take a look?
Matt Falkenhagen (Gerrit)
Jun 22, 2017, 4:04:08 AM6/22/17
to Makoto Shimazu, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org
Matt Falkenhagen would like Makoto Shimazu to review this change.
M content/renderer/render_thread_impl.cc
M content/renderer/service_worker/embedded_worker_instance_client_impl.cc
M content/renderer/service_worker/embedded_worker_instance_client_impl.h
M content/renderer/service_worker/service_worker_context_client.cc
M content/renderer/service_worker/service_worker_context_client.h
M third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h
M tools/metrics/histograms/enums.xml
M tools/metrics/histograms/histograms.xml
15 files changed, 230 insertions(+), 36 deletions(-)
Kinuko Yasuda (Gerrit)
Jun 23, 2017, 12:58:14 AM6/23/17
to Matt Falkenhagen, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Commit Bot, Makoto Shimazu, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
Kinuko Yasuda posted comments on this change.
Patch set 4:
(2 comments)
File content/browser/service_worker/embedded_worker_instance.cc:
Patch Set #4, Line 288:
base::TimeTicks start_worker_sent_time() { return start_worker_sent_time_; }nit: const method
File content/renderer/service_worker/embedded_worker_instance_client_impl.h:
Patch Set #4, Line 99:
base::TimeTicks webkit_initialized_time_;Why do we use webkit_ here and blink_ there?
To view, visit change 544718. To unsubscribe, visit settings.
Matt Falkenhagen (Gerrit)
Jun 23, 2017, 2:51:16 AM6/23/17
to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Kinuko Yasuda, Commit Bot, Makoto Shimazu, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
Matt Falkenhagen posted comments on this change.
Patch set 6:Commit-Queue +1
Thank you, updated. I also updated to accounted for negative time measurements. PTAL.
(2 comments)
File content/browser/service_worker/embedded_worker_instance.cc:
Patch Set #4, Line 288:
base::TimeTicks start_worker_sent_time() const {nit: const method
Done
File content/renderer/service_worker/embedded_worker_instance_client_impl.h:
Patch Set #4, Line 99:
base::TimeTicks blink_initialized_time_;
Why do we use webkit_ here and blink_ there?
Couldn't decide which was better :)
Settled on Blink. Done.
To view, visit change 544718. To unsubscribe, visit settings.
Kinuko Yasuda (Gerrit)
Jun 23, 2017, 4:49:00 AM6/23/17
to Matt Falkenhagen, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Tsuyoshi Horo, Commit Bot, Makoto Shimazu, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
Kinuko Yasuda posted comments on this change.
Patch set 6:Code-Review +1
Matt Falkenhagen (Gerrit)
Jun 23, 2017, 4:54:31 AM6/23/17
to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Daniel Cheng, Jesse Doherty, Kinuko Yasuda, Tsuyoshi Horo, Commit Bot, Makoto Shimazu, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
Matt Falkenhagen posted comments on this change.
Patch set 6:
+dcheng: can you review mojom?
+jwd: can you review histograms?
Daniel Cheng (Gerrit)
Jun 23, 2017, 5:02:10 AM6/23/17
to Matt Falkenhagen, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Jesse Doherty, Kinuko Yasuda, Tsuyoshi Horo, Commit Bot, Makoto Shimazu, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
Daniel Cheng posted comments on this change.
Patch set 6:Code-Review +1
ipc lgtm
(2 comments)
File content/common/service_worker/embedded_worker.mojom:
// EmbeddedWorkerInstanceClient is the renderer-side ("Client") of
// EmbeddedWorkerInstanceHost. It allows control of a renderer-side
// embedded worker. The browser uses this interface to start, stop, and
// issue commands to the worker.Thanks for adding these comments!
File content/renderer/service_worker/service_worker_context_client.cc:
mojom::EmbeddedWorkerStartTimingPtr timing =
mojom::EmbeddedWorkerStartTiming::New();If you want, it's possible to do this in one line:
auto timing = mojom::EmbeddedWorkerStartTiming::New(
start_worker_received_time_, blink_initialized_time_);One of the disadvantages is that it's possible to mix up argument order though...
To view, visit change 544718. To unsubscribe, visit settings.
Makoto Shimazu (Gerrit)
Jun 23, 2017, 6:14:13 AM6/23/17
to Matt Falkenhagen, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Daniel Cheng, Jesse Doherty, Kinuko Yasuda, Tsuyoshi Horo, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
Makoto Shimazu posted comments on this change.
To view, visit change 544718. To unsubscribe, visit settings.
Matt Falkenhagen (Gerrit)
Jun 26, 2017, 4:14:04 AM6/26/17
to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Makoto Shimazu, Daniel Cheng, Jesse Doherty, Kinuko Yasuda, Tsuyoshi Horo, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
Matt Falkenhagen posted comments on this change.
Patch set 6:
jwd: ping
(2 comments)
// EmbeddedWorkerInstanceClient is the renderer-side ("Client") of
// EmbeddedWorkerInstanceHost. It allows control of a renderer-side
// embedded worker. The browser uses this interface to start, stop, and
// issue commands to the worker.Thanks for adding these comments!
Ack, though I mostly just added on to what was already there.
File content/renderer/service_worker/service_worker_context_client.cc:
mojom::EmbeddedWorkerStartTimingPtr timing =
mojom::EmbeddedWorkerStartTiming::New();If you want, it's possible to do this in one line:
Thanks, I'll leave it as is for now since I plan to add a bunch more timing variables later.
To view, visit change 544718. To unsubscribe, visit settings.
Matt Falkenhagen (Gerrit)
Jun 26, 2017, 9:44:55 PM6/26/17
to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Ilya Sherman, Makoto Shimazu, Daniel Cheng, Jesse Doherty, Kinuko Yasuda, Tsuyoshi Horo, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
timing out jwd, adding +isherman for histograms
Matt Falkenhagen (Gerrit)
Jun 27, 2017, 5:06:21 AM6/27/17
to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Ilya Sherman, Makoto Shimazu, Daniel Cheng, Jesse Doherty, Kinuko Yasuda, Tsuyoshi Horo, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
I just discovered yesterday was a holiday for some of the metrics team. Sorry. In any case can isherman@ or jwd@ please review this? Thanks!
Ilya Sherman (Gerrit)
Jun 27, 2017, 11:27:10 AM6/27/17
to Matt Falkenhagen, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Ilya Sherman, Makoto Shimazu, Daniel Cheng, Jesse Doherty, Kinuko Yasuda, Tsuyoshi Horo, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
Ilya Sherman posted comments on this change.
Patch set 6:
(3 comments)
File content/browser/service_worker/service_worker_metrics.cc:
nit: Please factor out a small wrapper for this, that's called along the lines of
RecordStartMessageLatencyType(CrossProcessTimeDelta::INACCURATE_CLOCK);
RecordSuffixedMediumTimeHistogram(
"EmbeddedWorkerInstance.Start.StartMessageLatency",
StartSituationToSuffix(situation), start_worker_message_latency);I'm not seeing where this is included in histograms.xml. Am I just overlooking it?
UMA_HISTOGRAM_BOOLEAN("EmbeddedWorkerInstance.Start.WaitedForRendererSetup",
false);nit: Could you please factor out a wrapper for this histogram as well? Each UMA_HISTOGRAM_* macro expands to a sizeable chunk of code, so it's nice to help the compiler avoid duplicating that. Also, it helps avoid divergent typos in the parallel code paths.
To view, visit change 544718. To unsubscribe, visit settings.
Ilya Sherman (Gerrit)
Jun 27, 2017, 11:28:09 AM6/27/17
to Matt Falkenhagen, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Ilya Sherman, Makoto Shimazu, Daniel Cheng, Jesse Doherty, Kinuko Yasuda, Tsuyoshi Horo, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
(1 comment)
Patch Set #6, Line 22:
Let me know if you have any better ideas.Please remove this line prior to landing this CL (unless you want future code viewers to offer belated suggestions =])
To view, visit change 544718. To unsubscribe, visit settings.
Jesse Doherty (Gerrit)
Jun 27, 2017, 5:19:15 PM6/27/17
to Matt Falkenhagen, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Ilya Sherman, Makoto Shimazu, Daniel Cheng, Kinuko Yasuda, Tsuyoshi Horo, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
Jesse Doherty posted comments on this change.
Patch set 6:Code-Review +1
Tsuyoshi Horo (Gerrit)
Jun 27, 2017, 9:01:31 PM6/27/17
to Matt Falkenhagen, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Jesse Doherty, Ilya Sherman, Makoto Shimazu, Daniel Cheng, Kinuko Yasuda, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
Tsuyoshi Horo posted comments on this change.
Patch set 6:Code-Review +1
I am sorry that I overlooked it.
Matt Falkenhagen (Gerrit)
Jun 28, 2017, 4:36:24 AM6/28/17
to Tsuyoshi Horo, Ilya Sherman, Makoto Shimazu, Jesse Doherty, Daniel Cheng, Kinuko Yasuda, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Darin Fisher, Taiju Tsuiki, chromium...@chromium.org, Hiroki Nakagawa, John Abd-El-Malek, Commit Bot, Michael Nordman, Adam Barth, Aaron Boodman
Matt Falkenhagen uploaded patch set #9 to this change.
service worker: Add UMA for start worker IPC latency.
The motivation is to get a bound on how much optimization can be done before
the start worker message is received. This includes work to startup a renderer
faster and off-main-thread start worker.
In order to try to distinguish between time spent in process creation/setup vs
main thread contention, I tried to estimate when the renderer is launched
and ready to receive messages via RenderThreadImpl::Init(). However
it looks like on browser startup, the renderer setup finishes quickly
(before start worker is sent) but there a huge message latency (500 ms).
I suspect this can never be super accurate anyway because the renderer
may have been initialized long ago, was suspended, and we need to wait
for it to resume.
The CL also sets the path for sending timing information from the renderer to
browser, so we can record the timing directly in the renderer and remove IPCs
we currently use to keep track of the start sequence on the browser-side.
Because these IPCs are affected by IO thread contention on the browser side,
they can be causing inaccurate timing information logged to UMA.
Bug: 561209
Change-Id: I4d000ba2a66c6a0576bb6c0eee21b35590db12d4
---
M content/browser/service_worker/embedded_worker_instance.cc
M content/browser/service_worker/embedded_worker_instance.h
M content/browser/service_worker/embedded_worker_test_helper.cc
M content/browser/service_worker/service_worker_metrics.cc
M content/browser/service_worker/service_worker_metrics.h
M content/browser/service_worker/service_worker_metrics_unittest.cc
M content/common/service_worker/embedded_worker.mojom
M content/renderer/render_thread_impl.cc
M content/renderer/service_worker/embedded_worker_instance_client_impl.cc
M content/renderer/service_worker/embedded_worker_instance_client_impl.h
M content/renderer/service_worker/service_worker_context_client.cc
M content/renderer/service_worker/service_worker_context_client.h
M third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h
M tools/metrics/histograms/enums.xml
M tools/metrics/histograms/histograms.xml
15 files changed, 315 insertions(+), 36 deletions(-)
To view, visit change 544718. To unsubscribe, visit settings.
Matt Falkenhagen (Gerrit)
Jun 28, 2017, 4:39:10 AM6/28/17
to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Tsuyoshi Horo, Jesse Doherty, Ilya Sherman, Makoto Shimazu, Daniel Cheng, Kinuko Yasuda, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
Matt Falkenhagen posted comments on this change.
Patch set 9:Commit-Queue +1
Thanks for the comments! isherman@ do you want to take another look? diff against patch set 7 is the new changes.
(5 comments)
Patch Set #6, Line 22:
The CL also sets the path for sending tim
Please remove this line prior to landing this CL (unless you want future co
Wouldn't really mind actually, but went ahead and deleted it :)
File content/browser/service_worker/service_worker_metrics.cc:
Patch Set #6, Line 800:
ted) {
nit: Please factor out a small wrapper for this, that's called along the li
Good idea, done.
const base::TimeDelta start_worker_message_latency =
start_timing->start_worker_received_time - start_worker_sent_time
I'm not seeing where this is included in histograms.xml. Am I just overloo
No, good catch. Done.
UMA_HISTOGRAM_MEDIUM_TIMES(
"EmbeddedWorkerInstance.S
nit: Could you please factor out a wrapper for this histogram as well? Eac
Sure, done. I think I was once bit by an unrelated thing where I reused the UMA_HISTOGRAM macro with a variable for the histogram name and it crashed because the histogram gets cached, so I tend to just write out UMA_HISTOGRAM with string literals every time (although I know logically it's just the histogram name that shouldn't be factored out to a variable).
File tools/metrics/histograms/histograms.xml:
Patch Set #9, Line 95479:
<affected-histogram name="EmbeddedWorkerInstance.Start.StartMessageLatency"/>pretty print or git-cl format ordered it this way... i'm just going to let it do that
To view, visit change 544718. To unsubscribe, visit settings.
Ilya Sherman (Gerrit)
Jun 28, 2017, 11:44:54 AM6/28/17
to Matt Falkenhagen, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Ilya Sherman, Tsuyoshi Horo, Jesse Doherty, Makoto Shimazu, Daniel Cheng, Kinuko Yasuda, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
Ilya Sherman posted comments on this change.
Patch set 9:Code-Review +1
Thanks!
(1 comment)
Patch Set #9, Line 95479:
<affected-histogram name="EmbeddedWorkerInstance.Start.StartMessageLatency"/>pretty print or git-cl format ordered it this way... i'm just going to let
How odd! Would you mind filing a bug, assigned to me?
To view, visit change 544718. To unsubscribe, visit settings.
Matt Falkenhagen (Gerrit)
Jun 28, 2017, 11:51:22 AM6/28/17
to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Ilya Sherman, Tsuyoshi Horo, Jesse Doherty, Makoto Shimazu, Daniel Cheng, Kinuko Yasuda, Commit Bot, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
Matt Falkenhagen posted comments on this change.
Patch set 9:Commit-Queue +2
Commit Bot (Gerrit)
Jun 28, 2017, 12:54:44 PM6/28/17
to Matt Falkenhagen, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Ilya Sherman, Tsuyoshi Horo, Jesse Doherty, Makoto Shimazu, Daniel Cheng, Kinuko Yasuda, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
Commit Bot merged this change.
Approvals:
Tsuyoshi Horo: Looks good to me
Daniel Cheng: Looks good to me
Kinuko Yasuda: Looks good to me
Ilya Sherman: Looks good to me
Jesse Doherty: Looks good to me
Makoto Shimazu: Looks good to me
Matt Falkenhagen: Commit
service worker: Add UMA for start worker IPC latency.
The motivation is to get a bound on how much optimization can be done before
the start worker message is received. This includes work to startup a renderer
faster and off-main-thread start worker.
In order to try to distinguish between time spent in process creation/setup vs
main thread contention, I tried to estimate when the renderer is launched
and ready to receive messages via RenderThreadImpl::Init(). However
it looks like on browser startup, the renderer setup finishes quickly
(before start worker is sent) but there a huge message latency (500 ms).
I suspect this can never be super accurate anyway because the renderer
may have been initialized long ago, was suspended, and we need to wait
for it to resume.
The CL also sets the path for sending timing information from the renderer to
browser, so we can record the timing directly in the renderer and remove IPCs
we currently use to keep track of the start sequence on the browser-side.
Because these IPCs are affected by IO thread contention on the browser side,
they can be causing inaccurate timing information logged to UMA.
Bug: 561209
Change-Id: I4d000ba2a66c6a0576bb6c0eee21b35590db12d4
Reviewed-on: https://chromium-review.googlesource.com/544718
Reviewed-by: Ilya Sherman <ishe...@chromium.org>
Reviewed-by: Kinuko Yasuda <kin...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Makoto Shimazu <shi...@chromium.org>
Reviewed-by: Jesse Doherty <j...@chromium.org>
Reviewed-by: Tsuyoshi Horo <ho...@chromium.org>
Commit-Queue: Matt Falkenhagen <fal...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#483027}
---
M content/browser/service_worker/embedded_worker_instance.cc
M content/browser/service_worker/embedded_worker_instance.h
M content/browser/service_worker/embedded_worker_test_helper.cc
M content/browser/service_worker/service_worker_metrics.cc
M content/browser/service_worker/service_worker_metrics.h
M content/browser/service_worker/service_worker_metrics_unittest.cc
M content/common/service_worker/embedded_worker.mojom
M content/renderer/render_thread_impl.cc
M content/renderer/service_worker/embedded_worker_instance_client_impl.cc
M content/renderer/service_worker/embedded_worker_instance_client_impl.h
M content/renderer/service_worker/service_worker_context_client.cc
M content/renderer/service_worker/service_worker_context_client.h
M third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h
M tools/metrics/histograms/enums.xml
M tools/metrics/histograms/histograms.xml
15 files changed, 315 insertions(+), 36 deletions(-)
Matt Falkenhagen (Gerrit)
Jun 28, 2017, 9:12:57 PM6/28/17
to Commit Bot, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, blink-work...@chromium.org, dari...@chromium.org, dglazko...@chromium.org, horo+...@chromium.org, jsbell+ser...@chromium.org, kinuko+ser...@chromium.org, kinuko...@chromium.org, mlamouri+wa...@chromium.org, qsr+...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Ilya Sherman, Tsuyoshi Horo, Jesse Doherty, Makoto Shimazu, Daniel Cheng, Kinuko Yasuda, Aaron Boodman, Adam Barth, chromium...@chromium.org, Darin Fisher, John Abd-El-Malek, Michael Nordman, Hiroki Nakagawa, Taiju Tsuiki
Matt Falkenhagen posted comments on this change.
Patch set 10:
Patch Set 9: Code-Review+1
(1 comment)
Thanks!
Done: https://bugs.chromium.org/p/chromium/issues/detail?id=737822
Reply all
Reply to author
Forward
0 new messages