Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
996013004:
Add metric measuring Feature Provider Static() length in the browser process. (Closed)
|
Created:
5 years, 9 months ago by rkaplow Modified:
5 years, 9 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, asvitkine+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd metric measuring Feature Provider Static() length in the browser process.
We are investigating the cost of ExtensionService::Init(), and want to see the effect of this static initialization, specifically in the browser process which affects startup.
BUG=454789
Committed: https://crrev.com/fca0f4dc562018d756a160d40aff6eda9629fdfb
Cr-Commit-Position: refs/heads/master@{#320668}
Patch Set 1 #
Total comments: 10
Patch Set 2 : remove typo #Patch Set 3 : longer comment #
Total comments: 4
Patch Set 4 : ilya comments #
Messages
Total messages: 19 (5 generated)
rkaplow@chromium.org changed reviewers: + asvitkine@chromium.org, kalman@chromium.org
asvitkine@chromium.org: Please review changes in metrics kalman@chromium.org: Please review changes in extensions/ I realize it's a bit ugly how I verify we are in browser process but I couldn't find a cleaner way. Let me know if there is a better API for this.
https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... File extensions/common/features/feature_provider.cc (right): https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... extensions/common/features/feature_provider.cc:35: TRACE_EVENT0("startup", "extensions::FeatureProvider::Static"); What does "startup" mean? https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... extensions/common/features/feature_provider.cc:52: // Measure time only for browser process. Why only browser process, does UMA only work from browser process I guess?
https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... File extensions/common/features/feature_provider.cc (right): https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... extensions/common/features/feature_provider.cc:35: TRACE_EVENT0("startup", "extensions::FeatureProvider::Static"); On 2015/03/13 19:08:05, kalman wrote: > What does "startup" mean? It's a "category" used in the tracing tool, like an annotation. This just gets called (on the browser thread) on startup, so it makes sense to add the marking. https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... extensions/common/features/feature_provider.cc:52: // Measure time only for browser process. On 2015/03/13 19:08:05, kalman wrote: > Why only browser process, does UMA only work from browser process I guess? No, it doesn't, and that's actually the problem. This method gets called by the renderer process a lot. From my limited experimentation it seems to: -gets called in the renderer process on startup -gets called by extensions (not sure if each one or not) -gets called every time you load a new page on the renderer process. My goal is to see how long this contributed to startup, and this is called synchronously on the browser process / UI thread. IF I measured every time, that information would get lost in the noise of all the renderer process events.
On 2015/03/13 19:19:15, rkaplow wrote: > https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... > File extensions/common/features/feature_provider.cc (right): > > https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... > extensions/common/features/feature_provider.cc:35: TRACE_EVENT0("startup", > "extensions::FeatureProvider::Static"); > On 2015/03/13 19:08:05, kalman wrote: > > What does "startup" mean? > > It's a "category" used in the tracing tool, like an annotation. > This just gets called (on the browser thread) on startup, so it makes sense to > add the marking. > > https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... > extensions/common/features/feature_provider.cc:52: // Measure time only for > browser process. > On 2015/03/13 19:08:05, kalman wrote: > > Why only browser process, does UMA only work from browser process I guess? > > > No, it doesn't, and that's actually the problem. > This method gets called by the renderer process a lot. > From my limited experimentation it seems to: > -gets called in the renderer process on startup > -gets called by extensions (not sure if each one or not) > -gets called every time you load a new page on the renderer process. > > > My goal is to see how long this contributed to startup, and this is called > synchronously on the browser process / UI thread. IF I measured every time, that > information would get lost in the noise of all the renderer process events. Yup, the change is what I would like to measure. Thanks!
robliao@chromium.org changed reviewers: + robliao@chromium.org
https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... File extensions/common/features/feature_provider.cc (right): https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... extensions/common/features/feature_provider.cc:1: BB// Copyright 2013 The Chromium Authors. All rights reserved. Remove the BB
On 2015/03/13 19:21:19, robliao wrote: > https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... > File extensions/common/features/feature_provider.cc (right): > > https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... > extensions/common/features/feature_provider.cc:1: BB// Copyright 2013 The > Chromium Authors. All rights reserved. > Remove the BB oops, last minute typo.
lgtm https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... File extensions/common/features/feature_provider.cc (right): https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... extensions/common/features/feature_provider.cc:35: TRACE_EVENT0("startup", "extensions::FeatureProvider::Static"); On 2015/03/13 19:19:15, rkaplow wrote: > On 2015/03/13 19:08:05, kalman wrote: > > What does "startup" mean? > > It's a "category" used in the tracing tool, like an annotation. > This just gets called (on the browser thread) on startup, so it makes sense to > add the marking. Ok, thanks. I guess it does happen on startup, though it's supposed to be lazy, just... always required pretty early on. https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... extensions/common/features/feature_provider.cc:52: // Measure time only for browser process. On 2015/03/13 19:19:15, rkaplow wrote: > On 2015/03/13 19:08:05, kalman wrote: > > Why only browser process, does UMA only work from browser process I guess? > > > No, it doesn't, and that's actually the problem. > This method gets called by the renderer process a lot. > From my limited experimentation it seems to: > -gets called in the renderer process on startup > -gets called by extensions (not sure if each one or not) > -gets called every time you load a new page on the renderer process. > > > My goal is to see how long this contributed to startup, and this is called > synchronously on the browser process / UI thread. IF I measured every time, that > information would get lost in the noise of all the renderer process events. Ok, thanks. Yes it needs to get read on every everything-startup. I was thinking of having a look at whether we can share this better, I'm glad you're looking into the effect it has. If it's significant that's interesting data. Anyway you might want to mention something like your last paragraph in the comment.
https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... File extensions/common/features/feature_provider.cc (right): https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... extensions/common/features/feature_provider.cc:1: BB// Copyright 2013 The Chromium Authors. All rights reserved. On 2015/03/13 19:21:19, robliao wrote: > Remove the BB Done. https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... extensions/common/features/feature_provider.cc:35: TRACE_EVENT0("startup", "extensions::FeatureProvider::Static"); On 2015/03/13 19:25:20, kalman wrote: > On 2015/03/13 19:19:15, rkaplow wrote: > > On 2015/03/13 19:08:05, kalman wrote: > > > What does "startup" mean? > > > > It's a "category" used in the tracing tool, like an annotation. > > This just gets called (on the browser thread) on startup, so it makes sense to > > add the marking. > > Ok, thanks. I guess it does happen on startup, though it's supposed to be lazy, > just... always required pretty early on. Right. I don't think there's a big downside, I think in practice it should generally get called lazily by something that occurs in startup (i.e. ComponentLoader LoadAll()). Because of this - having this show up if someone is specifically looking at startup traces seems like a positive. https://codereview.chromium.org/996013004/diff/1/extensions/common/features/f... extensions/common/features/feature_provider.cc:52: // Measure time only for browser process. On 2015/03/13 19:25:20, kalman wrote: > On 2015/03/13 19:19:15, rkaplow wrote: > > On 2015/03/13 19:08:05, kalman wrote: > > > Why only browser process, does UMA only work from browser process I guess? > > > > > > No, it doesn't, and that's actually the problem. > > This method gets called by the renderer process a lot. > > From my limited experimentation it seems to: > > -gets called in the renderer process on startup > > -gets called by extensions (not sure if each one or not) > > -gets called every time you load a new page on the renderer process. > > > > > > My goal is to see how long this contributed to startup, and this is called > > synchronously on the browser process / UI thread. IF I measured every time, > that > > information would get lost in the noise of all the renderer process events. > > Ok, thanks. Yes it needs to get read on every everything-startup. I was thinking > of having a look at whether we can share this better, I'm glad you're looking > into the effect it has. If it's significant that's interesting data. > > Anyway you might want to mention something like your last paragraph in the > comment. Done.
rkaplow@chromium.org changed reviewers: + isherman@chromium.org
Adding ilya in case he catches it before the weekend as asvitkine is OOO
LGTM % nits. It would be great if you could also expand the CL description to include some motivation for this change as well as a bug #. As-is, it's not clear to me as an outside observer why this particular metric is an interesting one to measure. https://codereview.chromium.org/996013004/diff/40001/extensions/common/featur... File extensions/common/features/feature_provider.cc (right): https://codereview.chromium.org/996013004/diff/40001/extensions/common/featur... extensions/common/features/feature_provider.cc:57: if (process_type == "") { nit: It's generally better to use std::string() rather than "". For example, if somebody were to ever come along and change the type of process_type to also be a char*, this comparison would fail. That particular failure mode is unlikely, I admit, but it's generally best not to rely on implicit conversions where they're not needed. https://codereview.chromium.org/996013004/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/996013004/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:8860: +<histogram name="Extensions.FeatureProviderStaticInitTime"> nit: Please add a units attribute.
Thanks. Updated the description. https://codereview.chromium.org/996013004/diff/40001/extensions/common/featur... File extensions/common/features/feature_provider.cc (right): https://codereview.chromium.org/996013004/diff/40001/extensions/common/featur... extensions/common/features/feature_provider.cc:57: if (process_type == "") { On 2015/03/13 22:25:18, Ilya Sherman wrote: > nit: It's generally better to use std::string() rather than "". For example, if > somebody were to ever come along and change the type of process_type to also be > a char*, this comparison would fail. That particular failure mode is unlikely, > I admit, but it's generally best not to rely on implicit conversions where > they're not needed. Done. https://codereview.chromium.org/996013004/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/996013004/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:8860: +<histogram name="Extensions.FeatureProviderStaticInitTime"> On 2015/03/13 22:25:18, Ilya Sherman wrote: > nit: Please add a units attribute. Done.
The CQ bit was checked by rkaplow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/996013004/#ps60001 (title: "ilya comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/996013004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/fca0f4dc562018d756a160d40aff6eda9629fdfb Cr-Commit-Position: refs/heads/master@{#320668} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
