Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2931633002:
Add a metric to check how often SiteInstances can reuse processes (Closed)
DescriptionAdd a metric to check how often SiteInstances can reuse processes
We introduced a mechanism that allows SiteInstances to try to reuse
RenderProcessHosts in https://codereview.chromium.org/2857213005. This
CL adds a metric to check how often
ProcessReusePolicy::REUSE_CHECKING_FRAMES_AND_NAVIGATIONS leads to the
reuse of an existing RenderPorcessHost.
BUG=705318
Review-Url: https://codereview.chromium.org/2931633002
Cr-Commit-Position: refs/heads/master@{#479407}
Committed: https://chromium.googlesource.com/chromium/src/+/014fa484cfd3972bfae6296776502369750a6ed2
Patch Set 1 #
Total comments: 9
Patch Set 2 : Rebase #Patch Set 3 : Addressed comments #
Total comments: 2
Patch Set 4 : Addressed comments #
Messages
Total messages: 26 (16 generated)
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add a metric to check how often SiteInstances can reuse processes We introduced a mechanism that allows SiteInstances to try to reuse RenderProcessHosts in https://codereview.chromium.org/2857213005. This CL adds a metric to check how often ProcessReusePolicy::REUSE_CHECKING_FRAMES_AND_NAVIGATIONS leads to the reuse of an existing RenderPorcessHost. BUG=705318 ========== to ========== Add a metric to check how often SiteInstances can reuse processes We introduced a mechanism that allows SiteInstances to try to reuse RenderProcessHosts in https://codereview.chromium.org/2857213005. This CL adds a metric to check how often ProcessReusePolicy::REUSE_CHECKING_FRAMES_AND_NAVIGATIONS leads to the reuse of an existing RenderPorcessHost. BUG=705318 ==========
clamy@chromium.org changed reviewers: + creis@chromium.org, isherman@chromium.org
@creis, isherman: PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2931633002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2931633002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:70544: +<histogram name="SiteInstance.ReusePendingOrComittedSiteCouldReuse" Optional nit: I'd add a dot between "Site" and "Could" https://codereview.chromium.org/2931633002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:70544: +<histogram name="SiteInstance.ReusePendingOrComittedSiteCouldReuse" It looks like you're adding a new top-level category name, "SiteInstance". This might be appropriate, but I wanted to ask: Is there an existing top-level category that it would make sense to record this metric under? I'm a bit protective of the global namespace =) https://codereview.chromium.org/2931633002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:70551: + false when they created a new on. nit: s/on/one
LGTM with nits. I'm curious to get the data. Keep in mind that alexmos@ just added another use of REUSE_PENDING_OR_COMMITTED_SITE for --isolate-origins in https://codereview.chromium.org/2920473005, but there won't be much use of that in the wild yet. We're also considering switching the --process-per-site logic over to it, which would get a lot of use. Just wanted to point it out, in case you're solely interested in the ServiceWorker case. https://codereview.chromium.org/2931633002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2931633002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_process_host_impl.cc:3082: UMA_HISTOGRAM_BOOLEAN("SiteInstance.ReusePendingOrComittedSiteCouldReuse", typo: Committed https://codereview.chromium.org/2931633002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2931633002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:70544: +<histogram name="SiteInstance.ReusePendingOrComittedSiteCouldReuse" On 2017/06/07 19:53:15, Ilya Sherman wrote: > It looks like you're adding a new top-level category name, "SiteInstance". This > might be appropriate, but I wanted to ask: Is there an existing top-level > category that it would make sense to record this metric under? I'm a bit > protective of the global namespace =) We don't have a ProcessModel category, which might make sense at some point. It's most similar to metrics we track in SiteIsolation (e.g., SiteIsolation.*ProcessCountEstimate) or Memory (e.g., Memory.RendererProcessCount). I could see using SiteIsolation here, even though it's not strictly about that. On 2017/06/07 19:53:15, Ilya Sherman wrote: > Optional nit: I'd add a dot between "Site" and "Could" +1
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2931633002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2931633002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_process_host_impl.cc:3082: UMA_HISTOGRAM_BOOLEAN("SiteInstance.ReusePendingOrComittedSiteCouldReuse", On 2017/06/07 20:15:33, Charlie Reis wrote: > typo: Committed Done. https://codereview.chromium.org/2931633002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2931633002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:70544: +<histogram name="SiteInstance.ReusePendingOrComittedSiteCouldReuse" On 2017/06/07 19:53:15, Ilya Sherman wrote: > Optional nit: I'd add a dot between "Site" and "Could" Done. https://codereview.chromium.org/2931633002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:70544: +<histogram name="SiteInstance.ReusePendingOrComittedSiteCouldReuse" On 2017/06/07 20:15:33, Charlie Reis wrote: > On 2017/06/07 19:53:15, Ilya Sherman wrote: > > It looks like you're adding a new top-level category name, "SiteInstance". > This > > might be appropriate, but I wanted to ask: Is there an existing top-level > > category that it would make sense to record this metric under? I'm a bit > > protective of the global namespace =) > > We don't have a ProcessModel category, which might make sense at some point. > It's most similar to metrics we track in SiteIsolation (e.g., > SiteIsolation.*ProcessCountEstimate) or Memory (e.g., > Memory.RendererProcessCount). > > I could see using SiteIsolation here, even though it's not strictly about that. > > On 2017/06/07 19:53:15, Ilya Sherman wrote: > > Optional nit: I'd add a dot between "Site" and "Could" > > +1 I have put the histogram in SiteIsolation. https://codereview.chromium.org/2931633002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:70551: + false when they created a new on. On 2017/06/07 19:53:15, Ilya Sherman wrote: > nit: s/on/one Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM % a comment: https://codereview.chromium.org/2931633002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2931633002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:71119: + enum="BooleanEnabled"> Please define a custom enum in enums.xml with labels appropriate for this histogram.
Thanks! https://codereview.chromium.org/2931633002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2931633002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:71119: + enum="BooleanEnabled"> On 2017/06/13 21:39:13, Ilya Sherman wrote: > Please define a custom enum in enums.xml with labels appropriate for this > histogram. Done.
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2931633002/#ps60001 (title: "Addressed comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497453636580910,
"parent_rev": "10b7639ad904a2969a39781bb782118020e1fe9d", "commit_rev":
"014fa484cfd3972bfae6296776502369750a6ed2"}
Message was sent while issue was closed.
Description was changed from ========== Add a metric to check how often SiteInstances can reuse processes We introduced a mechanism that allows SiteInstances to try to reuse RenderProcessHosts in https://codereview.chromium.org/2857213005. This CL adds a metric to check how often ProcessReusePolicy::REUSE_CHECKING_FRAMES_AND_NAVIGATIONS leads to the reuse of an existing RenderPorcessHost. BUG=705318 ========== to ========== Add a metric to check how often SiteInstances can reuse processes We introduced a mechanism that allows SiteInstances to try to reuse RenderProcessHosts in https://codereview.chromium.org/2857213005. This CL adds a metric to check how often ProcessReusePolicy::REUSE_CHECKING_FRAMES_AND_NAVIGATIONS leads to the reuse of an existing RenderPorcessHost. BUG=705318 Review-Url: https://codereview.chromium.org/2931633002 Cr-Commit-Position: refs/heads/master@{#479407} Committed: https://chromium.googlesource.com/chromium/src/+/014fa484cfd3972bfae629677650... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/014fa484cfd3972bfae629677650... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
