Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2697323004:
Adds UMA metrics for renderer uptime (Closed)
DescriptionAdds UMA metrics for renderer uptime
Adds UMA metrics for renderer uptime to understand memory usage of long running Chrome.
This adds two metrics:
Memory.Experimental.Renderer.Uptime logs uptime of a render process.
Memory.Experimental.Renderer.LoadsInMainFrameDuringUptime logs the number of main frame page loads that happen in the lifetime of a render process.
BUG=693524
Review-Url: https://codereview.chromium.org/2697323004
Cr-Commit-Position: refs/heads/master@{#455670}
Committed: https://chromium.googlesource.com/chromium/src/+/2c94c4d78f44069ccca52395105f284f51bcf028
Patch Set 1 #
Total comments: 4
Patch Set 2 : fix #
Total comments: 13
Patch Set 3 : Removed metric provider #
Total comments: 10
Patch Set 4 : fix #Patch Set 5 : fix #
Total comments: 1
Patch Set 6 : fix #
Total comments: 7
Patch Set 7 : fix #Patch Set 8 : fix #
Messages
Total messages: 47 (28 generated)
Description was changed from ========== Add UMA metrics for renderer uptime Add UMA metrics for renderer uptime to understand memory usage of long running Chrome. BUG= ========== to ========== Add UMA metrics for renderer uptime Add UMA metrics for renderer uptime to understand memory usage of long running Chrome. BUG=693524 ==========
Description was changed from ========== Add UMA metrics for renderer uptime Add UMA metrics for renderer uptime to understand memory usage of long running Chrome. BUG=693524 ========== to ========== Adds UMA metrics for renderer uptime Adds UMA metrics for renderer uptime to understand memory usage of long running Chrome. This adds two metrics: Memory.Experimental.Renderer.Uptime logs uptime of a render process. Memory.Experimental.Renderer.LoadsInMainFrameDuringUptime logs the number of main frame page loads that happen in the lifetime of a render process. BUG=693524 ==========
keishi@chromium.org changed reviewers: + sky@chromium.org
PTAL. I would like to add UMA metrics that records renderer process uptime to understand memory of a long running render process.
sky@chromium.org changed reviewers: + mpearson@chromium.org
+mpearson You need mpearson for the histogram change, and as mpearson is also an owner of c/b/metrics I'm letting him review the changes there (sorry Mark). This means you only need me for c/b/ui/tab_helpers and c/b/BUILD.gn, which LGTM
Comments on histograms.xml below. I'm very slow at core chrome/browser/metrics/ code reviews. If you're comfortable with that, I can do the review. Frankly though I'd rather if you can poke around and find metrics code that does something similar, see who touched it last, and ask that person to do the review. It'll probably go smoother and faster then. --mark https://codereview.chromium.org/2697323004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2697323004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:27337: + (excludes extensions). Please add Emitted when the processes quits if true. Otherwise, please explain. Also, if it's emitted when the process quits, how does the fast quitting path affect things? Is it still emitted? https://codereview.chromium.org/2697323004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:27343: + <summary>The uptime of a render process (excludes extensions).</summary> 1. See https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... 2. Please clarify what uptime means. Is this measured by something like timeticks (which pauses when the computer is asleep) or wall time (which does not)?
keishi@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine@ I see you reviewed some changes to desktop_session_duration which I based this CL on. May I ask you to take a look? https://codereview.chromium.org/2697323004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2697323004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:27337: + (excludes extensions). On 2017/02/17 19:37:11, Mark P wrote: > Please add > Emitted when the processes quits > if true. > > Otherwise, please explain. > > Also, if it's emitted when the process quits, how does the fast quitting path > affect things? Is it still emitted? Done. https://codereview.chromium.org/2697323004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:27343: + <summary>The uptime of a render process (excludes extensions).</summary> On 2017/02/17 19:37:11, Mark P wrote: > 1. See > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... > > 2. Please clarify what uptime means. Is this measured by something like > timeticks (which pauses when the computer is asleep) or wall time (which does > not)? Done.
keishi@chromium.org changed reviewers: + isherman@chromium.org
Its been a while but I haven't been able to get this reviewed. +isherman Could you please take a look instead? Thanks.
asvitkine@chromium.org changed reviewers: - isherman@chromium.org
Sorry for the delay reviewing - I'll take a look. In the future, feel free to ping reviewers - either in the thread or via IM if they're being unresponsive. isherman to cc
https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/renderer_uptime_metrics_provider.cc (right): https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_metrics_provider.cc:24: registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CREATED, It's strange to use a provider class if you're not overriding any of its provider interfaces. Are you only doing this so that this observing would be turned on and off based on UMA enabled state? If so, please don't. We specifically do not want to have any performance optimizations based on UMA state - as this would result in UMA users getting a worse experience than non-UMA users and would provide an incentive for users to opt out of UMA, which we do not want to exist. So, with that in mind, I don't think you need to use the provider class at all. You just need to always listen for these notifications - which can be done entirely in RendererUptimeTracker. https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/renderer_uptime_tracker.cc (right): https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_tracker.cc:16: void RendererUptimeTracker::Initialize() { DCHECK(!g_instance); https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_tracker.cc:45: base::TimeDelta::FromDays(7), 100); We strongly discourage 100 bucket histograms now. Can you use 50 instead? https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_tracker.cc:55: if (it != info_map_.end()) What does it mean if it's not found? Should it be a DCHECK()? Or if it's a valid case, add a comment explaining. https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/renderer_uptime_web_contents_observer.cc (right): https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_web_contents_observer.cc:25: if (!::RendererUptimeTracker::IsInitialized()) Nit: No :: Remove from later in the file as well. https://codereview.chromium.org/2697323004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2697323004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27333: +<histogram name="Memory.Experimental.Renderer.LoadsInMainFrameDuringUptime"> Nit: Add units=loads
mpearson@chromium.org changed reviewers: - mpearson@chromium.org
https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/renderer_uptime_metrics_provider.cc (right): https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_metrics_provider.cc:24: registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CREATED, On 2017/02/28 16:13:11, Alexei Svitkine (slow) wrote: > It's strange to use a provider class if you're not overriding any of its > provider interfaces. > > Are you only doing this so that this observing would be turned on and off based > on UMA enabled state? > > If so, please don't. We specifically do not want to have any performance > optimizations based on UMA state - as this would result in UMA users getting a > worse experience than non-UMA users and would provide an incentive for users to > opt out of UMA, which we do not want to exist. > > So, with that in mind, I don't think you need to use the provider class at all. > You just need to always listen for these notifications - which can be done > entirely in RendererUptimeTracker. Done. Removed metric provider class. FYI: Maybe it has a good reason to, but I copied this code from ChromeStabilityMetricsProvider::OnRecordingEnabled. https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/renderer_uptime_tracker.cc (right): https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_tracker.cc:16: void RendererUptimeTracker::Initialize() { On 2017/02/28 16:13:12, Alexei Svitkine (slow) wrote: > DCHECK(!g_instance); Done. https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_tracker.cc:45: base::TimeDelta::FromDays(7), 100); On 2017/02/28 16:13:12, Alexei Svitkine (slow) wrote: > We strongly discourage 100 bucket histograms now. Can you use 50 instead? Done. https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_tracker.cc:55: if (it != info_map_.end()) On 2017/02/28 16:13:11, Alexei Svitkine (slow) wrote: > What does it mean if it's not found? Should it be a DCHECK()? Or if it's a valid > case, add a comment explaining. Done. Changed to DCHECK. I was unsure if metrics turn on/off dynamically. https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/renderer_uptime_web_contents_observer.cc (right): https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_web_contents_observer.cc:25: if (!::RendererUptimeTracker::IsInitialized()) On 2017/02/28 16:13:12, Alexei Svitkine (slow) wrote: > Nit: No :: > > Remove from later in the file as well. Done. https://codereview.chromium.org/2697323004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2697323004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:27333: +<histogram name="Memory.Experimental.Renderer.LoadsInMainFrameDuringUptime"> On 2017/02/28 16:13:12, Alexei Svitkine (slow) wrote: > Nit: Add units=loads Done.
The CQ bit was checked by keishi@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/renderer_uptime_metrics_provider.cc (right): https://codereview.chromium.org/2697323004/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_metrics_provider.cc:24: registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CREATED, On 2017/03/01 08:09:54, keishi wrote: > On 2017/02/28 16:13:11, Alexei Svitkine (slow) wrote: > > It's strange to use a provider class if you're not overriding any of its > > provider interfaces. > > > > Are you only doing this so that this observing would be turned on and off > based > > on UMA enabled state? > > > > If so, please don't. We specifically do not want to have any performance > > optimizations based on UMA state - as this would result in UMA users getting a > > worse experience than non-UMA users and would provide an incentive for users > to > > opt out of UMA, which we do not want to exist. > > > > So, with that in mind, I don't think you need to use the provider class at > all. > > You just need to always listen for these notifications - which can be done > > entirely in RendererUptimeTracker. > > Done. Removed metric provider class. > > FYI: Maybe it has a good reason to, but I copied this code from > ChromeStabilityMetricsProvider::OnRecordingEnabled. Thanks for pointing that out. I filed crbug.com/697461 for us to fix this. One difference is that code also ends up saving state to prefs - which is awkward because the code to clear those prefs is the one that actual uploads the log - so with the current set up they wouldn't be cleared I think. In this case, you're just using histograms so it doesn't have that concern. https://codereview.chromium.org/2697323004/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/renderer_uptime_tracker.cc (right): https://codereview.chromium.org/2697323004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_tracker.cc:89: case content::NOTIFICATION_RENDERER_PROCESS_TERMINATED: { Are you forgetting to a register for this? https://codereview.chromium.org/2697323004/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/renderer_uptime_tracker.h (right): https://codereview.chromium.org/2697323004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_tracker.h:19: static void Initialize(); Where is this called? I think it should be called early during start up - so that you register for notifications early and don't miss anything. https://codereview.chromium.org/2697323004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_tracker.h:23: static bool IsInitialized(); Any reason to have this since there's already a Get() function? https://codereview.chromium.org/2697323004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_tracker.h:30: void OnLoadInMainFrame(int pid); These can now be private, right? https://codereview.chromium.org/2697323004/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/renderer_uptime_web_contents_observer.cc (right): https://codereview.chromium.org/2697323004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_web_contents_observer.cc:38: ::RendererUptimeTracker::Get()->OnLoadInMainFrame( Nit: No ::
https://codereview.chromium.org/2697323004/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/renderer_uptime_tracker.cc (right): https://codereview.chromium.org/2697323004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_tracker.cc:89: case content::NOTIFICATION_RENDERER_PROCESS_TERMINATED: { On 2017/03/01 15:50:18, Alexei Svitkine (slow) wrote: > Are you forgetting to a register for this? Done. https://codereview.chromium.org/2697323004/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/renderer_uptime_tracker.h (right): https://codereview.chromium.org/2697323004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_tracker.h:19: static void Initialize(); On 2017/03/01 15:50:18, Alexei Svitkine (slow) wrote: > Where is this called? I think it should be called early during start up - so > that you register for notifications early and don't miss anything. The first RenderProcessHost instances seem to be created in PreMainMessageLoopRun so I would like to call this before that. I have placed it next to DesktopSessionDurationTracker::Initialize(). https://codereview.chromium.org/2697323004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_tracker.h:23: static bool IsInitialized(); On 2017/03/01 15:50:18, Alexei Svitkine (slow) wrote: > Any reason to have this since there's already a Get() function? Sorry. Removed. https://codereview.chromium.org/2697323004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_tracker.h:30: void OnLoadInMainFrame(int pid); On 2017/03/01 15:50:18, Alexei Svitkine (slow) wrote: > These can now be private, right? Done. Moved private methods to private. https://codereview.chromium.org/2697323004/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/renderer_uptime_web_contents_observer.cc (right): https://codereview.chromium.org/2697323004/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_web_contents_observer.cc:38: ::RendererUptimeTracker::Get()->OnLoadInMainFrame( On 2017/03/01 15:50:18, Alexei Svitkine (slow) wrote: > Nit: No :: Done. https://codereview.chromium.org/2697323004/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/renderer_uptime_tracker.cc (right): https://codereview.chromium.org/2697323004/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/renderer_uptime_tracker.cc:92: case content::NOTIFICATION_RENDERER_PROCESS_CLOSED: { Added. This seems to be called in case of a crash. The process is gone but the RenderProcessHost may be reused.
The CQ bit was checked by keishi@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...
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_...)
The CQ bit was checked by keishi@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...
https://codereview.chromium.org/2697323004/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/renderer_uptime_tracker.cc (right): https://codereview.chromium.org/2697323004/diff/100001/chrome/browser/metrics... chrome/browser/metrics/renderer_uptime_tracker.cc:71: if (it != info_map_.end()) { Turns out, for in process browser tests, this gets called without the render process created/terminated notifications. So I had to change it back from a DCHECK.
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 % comments https://codereview.chromium.org/2697323004/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/renderer_uptime_tracker.h (right): https://codereview.chromium.org/2697323004/diff/100001/chrome/browser/metrics... chrome/browser/metrics/renderer_uptime_tracker.h:26: void OnLoadInMainFrame(int pid); Add a comment. https://codereview.chromium.org/2697323004/diff/100001/chrome/browser/metrics... chrome/browser/metrics/renderer_uptime_tracker.h:37: void OnRendererStarted(int pid); Why protected and not private? https://codereview.chromium.org/2697323004/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/renderer_uptime_web_contents_observer.h (right): https://codereview.chromium.org/2697323004/diff/100001/chrome/browser/metrics... chrome/browser/metrics/renderer_uptime_web_contents_observer.h:20: content::WebContents* web_contents); I'm not sure this should be public.
https://codereview.chromium.org/2697323004/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/renderer_uptime_tracker.h (right): https://codereview.chromium.org/2697323004/diff/100001/chrome/browser/metrics... chrome/browser/metrics/renderer_uptime_tracker.h:26: void OnLoadInMainFrame(int pid); On 2017/03/02 16:12:24, Alexei Svitkine (very slow) wrote: > Add a comment. Done. https://codereview.chromium.org/2697323004/diff/100001/chrome/browser/metrics... chrome/browser/metrics/renderer_uptime_tracker.h:37: void OnRendererStarted(int pid); On 2017/03/02 16:12:24, Alexei Svitkine (very slow) wrote: > Why protected and not private? Done. https://codereview.chromium.org/2697323004/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/renderer_uptime_web_contents_observer.h (right): https://codereview.chromium.org/2697323004/diff/100001/chrome/browser/metrics... chrome/browser/metrics/renderer_uptime_web_contents_observer.h:20: content::WebContents* web_contents); On 2017/03/02 16:12:24, Alexei Svitkine (very slow) wrote: > I'm not sure this should be public. Moved to private.
The CQ bit was checked by keishi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2697323004/#ps140001 (title: "fix")
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by keishi@chromium.org
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by keishi@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": 140001, "attempt_start_ts": 1489033530913710,
"parent_rev": "8e621e208f82c7136a6dc8f32e0db7c6df00685d", "commit_rev":
"2c94c4d78f44069ccca52395105f284f51bcf028"}
Message was sent while issue was closed.
Description was changed from ========== Adds UMA metrics for renderer uptime Adds UMA metrics for renderer uptime to understand memory usage of long running Chrome. This adds two metrics: Memory.Experimental.Renderer.Uptime logs uptime of a render process. Memory.Experimental.Renderer.LoadsInMainFrameDuringUptime logs the number of main frame page loads that happen in the lifetime of a render process. BUG=693524 ========== to ========== Adds UMA metrics for renderer uptime Adds UMA metrics for renderer uptime to understand memory usage of long running Chrome. This adds two metrics: Memory.Experimental.Renderer.Uptime logs uptime of a render process. Memory.Experimental.Renderer.LoadsInMainFrameDuringUptime logs the number of main frame page loads that happen in the lifetime of a render process. BUG=693524 Review-Url: https://codereview.chromium.org/2697323004 Cr-Commit-Position: refs/heads/master@{#455670} Committed: https://chromium.googlesource.com/chromium/src/+/2c94c4d78f44069ccca52395105f... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/2c94c4d78f44069ccca52395105f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
