Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2566043004:
Add renderer memory metrics (Closed)
DescriptionAdd renderer memory metrics
We will collect per allocator memory usage metrics for the renderer at DidFinishLoad and Shutdown.
GetRendererMemoryMetrics generalizes the memory info collection that was used in RecordPurgeAndSuspendMetrics for use with my new metrics.
GetRendererMemoryMetrics is in RenderThreadImpl because it accesses discardable_shared_memory_manager_.
BUG=
Review-Url: https://codereview.chromium.org/2566043004
Cr-Commit-Position: refs/heads/master@{#443195}
Committed: https://chromium.googlesource.com/chromium/src/+/51ed0d54153a164e68155447fcd1cfcb393f1870
Patch Set 1 #
Total comments: 4
Patch Set 2 : Added per RenderView size #Patch Set 3 : Changed to MB #Patch Set 4 : Moved to content #
Total comments: 2
Patch Set 5 : Removed Shutdown, added MainFrameDidFinishLoad, used suffix #
Total comments: 9
Patch Set 6 : Added RendererMemoryAllocator suffix #
Total comments: 6
Patch Set 7 : Fixed nits #Patch Set 8 : Fixed test failure #
Messages
Total messages: 35 (14 generated)
Description was changed from ========== Add renderer memory metrics BUG= ========== to ========== Add renderer memory metrics We will collect per allocator memory usage metrics for the renderer at DidFinishLoad and Shutdown. GetRendererMemoryMetrics generalizes the memory info collection that was used in RecordPurgeAndSuspendMetrics for use with my new metrics. GetRendererMemoryMetrics is in RenderThreadImpl because it accesses discardable_shared_memory_manager_. BUG= ==========
keishi@chromium.org changed reviewers: + haraken@chromium.org, sky@chromium.org, tasak@google.com
I am writing a document on this but I think the biggest problems with the current memory usage UMAs are: - Shared memory size is not tracked - Peak memory usage is under represented - No data about per-allocator memory usage primiano@ is working to separate memory-infra from tracing and that will enable us to have very good UMAs to address these issues. But in the meantime I would like to start by adding these two metrics. - Renderer’s per allocator allocations (using tasak’s hack) for DidFinishLoad and ProcessShutdown. This CL - Peak memory usage relative to physical memory https://codereview.chromium.org/2566083002/
haraken@chromium.org changed reviewers: + primiano@chromium.org
non-owner LGTM It will take months to figure out how to collect consistent UMAs from the renderer. So, +1 to starting experiment like this in parallel with the work of implementing a right infrastructure to collect memory-infra-based UMAs. https://codereview.chromium.org/2566043004/diff/1/chrome/renderer/chrome_rend... File chrome/renderer/chrome_render_frame_observer.cc (right): https://codereview.chromium.org/2566043004/diff/1/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_frame_observer.cc:255: content::RenderThread::Get()->GetRendererMemoryMetrics(&memory_metrics); Can we measure # of tabs in this renderer at this point? If there're many cases that this renderer is shared by multiple tabs, we'll end up with collecting "meaningless" values -- we need to think about another way to make the measurement a more meaningful thing. https://codereview.chromium.org/2566043004/diff/1/chrome/renderer/chrome_rend... File chrome/renderer/chrome_render_thread_observer.cc (right): https://codereview.chromium.org/2566043004/diff/1/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_thread_observer.cc:308: UMA_HISTOGRAM_MEMORY_MB( I might prefer unifying the units to MB though. It's confusing some allocators report MB whereas others report KB.
On 2016/12/12 11:32:27, haraken wrote: > non-owner LGTM > > It will take months to figure out how to collect consistent UMAs from the > renderer. So, +1 to starting experiment like this in parallel with the work of > implementing a right infrastructure to collect memory-infra-based UMAs. > > https://codereview.chromium.org/2566043004/diff/1/chrome/renderer/chrome_rend... > File chrome/renderer/chrome_render_frame_observer.cc (right): > > https://codereview.chromium.org/2566043004/diff/1/chrome/renderer/chrome_rend... > chrome/renderer/chrome_render_frame_observer.cc:255: > content::RenderThread::Get()->GetRendererMemoryMetrics(&memory_metrics); > > Can we measure # of tabs in this renderer at this point? > > If there're many cases that this renderer is shared by multiple tabs, we'll end > up with collecting "meaningless" values -- we need to think about another way to > make the measurement a more meaningful thing. > > https://codereview.chromium.org/2566043004/diff/1/chrome/renderer/chrome_rend... > File chrome/renderer/chrome_render_thread_observer.cc (right): > > https://codereview.chromium.org/2566043004/diff/1/chrome/renderer/chrome_rend... > chrome/renderer/chrome_render_thread_observer.cc:308: UMA_HISTOGRAM_MEMORY_MB( > > I might prefer unifying the units to MB though. It's confusing some allocators > report MB whereas others report KB. I think this is a really good idea, but we should also emit these metrics every time we emit Memory.Total2, etc. [which I believe is with every UMA upload]. That way we can compare these numbers against our existing numbers and better understand how different sampling times affect our results.
non-owner LGTM % erik's comment about units.
keishi@chromium.org changed reviewers: + jam@chromium.org - sky@chromium.org
jam@ would you please take a look? Thanks. https://codereview.chromium.org/2566043004/diff/1/chrome/renderer/chrome_rend... File chrome/renderer/chrome_render_frame_observer.cc (right): https://codereview.chromium.org/2566043004/diff/1/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_frame_observer.cc:255: content::RenderThread::Get()->GetRendererMemoryMetrics(&memory_metrics); On 2016/12/12 11:32:27, haraken wrote: > > Can we measure # of tabs in this renderer at this point? > > If there're many cases that this renderer is shared by multiple tabs, we'll end > up with collecting "meaningless" values -- we need to think about another way to > make the measurement a more meaningful thing. Added Memory.Experimental.Renderer.*.TotalAllocatedPerRenderViewMB which divides the memory size with the # of RenderViews. https://codereview.chromium.org/2566043004/diff/1/chrome/renderer/chrome_rend... File chrome/renderer/chrome_render_thread_observer.cc (right): https://codereview.chromium.org/2566043004/diff/1/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_thread_observer.cc:308: UMA_HISTOGRAM_MEMORY_MB( On 2016/12/12 11:32:27, haraken wrote: > > I might prefer unifying the units to MB though. It's confusing some allocators > report MB whereas others report KB. > Unified new UMAs to MB. Leaving the existing PurgeAndSuspend UMAs as is because we don't want to split the data.
why are you adding a public api to content so that chrome can call UM? content makes UMA calls directly.
On 2017/01/06 17:05:01, jam wrote: > why are you adding a public api to content so that chrome can call UM? content > makes UMA calls directly. Moved UMA calls to content.
keishi@chromium.org changed reviewers: + isherman@chromium.org
isherman@ could you review histograms.xml? Thanks.
lgtm
Please use histogram_suffixes elements to reduce the amount of repetition in the histograms.xml file. https://codereview.chromium.org/2566043004/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2566043004/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:940: void RenderThreadImpl::Shutdown() { Are you sure that this code is routinely reached? My understanding is that renderers typically go through a fast teardown path, which skips pretty much all of the cleanup work.
I removed the metrics for Shutdown because they weren't called. Instead I added MainFrameDidFinishLoad metrics which only records data on main frame DidFinishLoad timings, and used histogram_suffix. https://codereview.chromium.org/2566043004/diff/60001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2566043004/diff/60001/content/renderer/render... content/renderer/render_thread_impl.cc:940: void RenderThreadImpl::Shutdown() { On 2017/01/10 20:29:56, Ilya Sherman wrote: > Are you sure that this code is routinely reached? My understanding is that > renderers typically go through a fast teardown path, which skips pretty much all > of the cleanup work. You're right this wasn't called so I've removed this.
Thanks. https://codereview.chromium.org/2566043004/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2566043004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26701: +<histogram name="Memory.Experimental.Renderer.BlinkGCMB" units="MB"> Optional nit: Are you sure that you want "MB" to be included in the metric name? It's already present in the units attribute, and IMO it makes the metric name harder to read. https://codereview.chromium.org/2566043004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26706: +</histogram> To clarify, I'd suggest using histogram suffixes roughly like so: <histogram name="Memory.Experimental.Renderer" base="true" units="MB"> <owner>keishi@chromium.org</owner> <summary> The renderer process's memory usage after a page load. </summary> </histogram> <histogram_suffixes name="RendererMemoryAllocator" separator="."> <suffix name="BlinkGCMB" label="Constrained to the BlinkGC allocator"> ... <suffix name="TotalAllocatedMB" label="Summed over the PartitionAlloc, malloc, discardable memory, mainThreadIsolate() and BlinkGC allocators"> ... <affected-histogram name="Memory.Experimental.Renderer"> </histogram_suffixes> (as well as the histogram_suffixes element that you already have) https://codereview.chromium.org/2566043004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:115109: + <suffix name="DidFinishLoad" label="DidFinishLoad"/> nit: I *think* this suffix is recorded every time a frame on the page reaches DidFinishLoad(). Is that correct? If so, please clarify this in the label. Also, I suspect that this might be confusing to interpret -- if a page has 4 frames, it will emit to this metric 4 times. That will bias this metric towards pages with more frames. Is that desirable? https://codereview.chromium.org/2566043004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:115110: + <suffix name="MainFrameDidFinishLoad" label="Main frame DidFinishLoad"/> nit: Please move these suffixes above all of the affected-histogram entries.
https://codereview.chromium.org/2566043004/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2566043004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26701: +<histogram name="Memory.Experimental.Renderer.BlinkGCMB" units="MB"> On 2017/01/12 01:04:14, Ilya Sherman wrote: > Optional nit: Are you sure that you want "MB" to be included in the metric name? > It's already present in the units attribute, and IMO it makes the metric name > harder to read. Done. Removed MB from the names. https://codereview.chromium.org/2566043004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26701: +<histogram name="Memory.Experimental.Renderer.BlinkGCMB" units="MB"> On 2017/01/12 01:04:14, Ilya Sherman wrote: > Optional nit: Are you sure that you want "MB" to be included in the metric name? > It's already present in the units attribute, and IMO it makes the metric name > harder to read. Done. Removed MB from names. https://codereview.chromium.org/2566043004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26706: +</histogram> On 2017/01/12 01:04:14, Ilya Sherman wrote: > To clarify, I'd suggest using histogram suffixes roughly like so: > > <histogram name="Memory.Experimental.Renderer" base="true" units="MB"> > <mailto:owner>keishi@chromium.org</owner> > <summary> > The renderer process's memory usage after a page load. > </summary> > </histogram> > > <histogram_suffixes name="RendererMemoryAllocator" separator="."> > <suffix name="BlinkGCMB" label="Constrained to the BlinkGC allocator"> > ... > <suffix name="TotalAllocatedMB" > label="Summed over the PartitionAlloc, malloc, discardable memory, > mainThreadIsolate() and BlinkGC allocators"> > ... > <affected-histogram name="Memory.Experimental.Renderer"> > </histogram_suffixes> > > (as well as the histogram_suffixes element that you already have) Done. https://codereview.chromium.org/2566043004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:115109: + <suffix name="DidFinishLoad" label="DidFinishLoad"/> On 2017/01/12 01:04:14, Ilya Sherman wrote: > nit: I *think* this suffix is recorded every time a frame on the page reaches > DidFinishLoad(). Is that correct? If so, please clarify this in the label. > Also, I suspect that this might be confusing to interpret -- if a page has 4 > frames, it will emit to this metric 4 times. That will bias this metric towards > pages with more frames. Is that desirable? Done. Main frame only will be more stable but iframes like ads may be large memory consumers so I want to have that data too. https://codereview.chromium.org/2566043004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:115110: + <suffix name="MainFrameDidFinishLoad" label="Main frame DidFinishLoad"/> On 2017/01/12 01:04:14, Ilya Sherman wrote: > nit: Please move these suffixes above all of the affected-histogram entries. Done.
Thanks. Metrics LGTM % remaining nits: https://codereview.chromium.org/2566043004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2566043004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:115231: + <suffix name="BlinkGC" label="Constrained to the BlinkGC allocator"/> nit: Please add base="true" for all of these suffixes as well. base="true" just means there's no histogram named "Memory.Experimental.Renderer.BlinkGC" -- it's used a base name that generates actual histogram names via suffixes. https://codereview.chromium.org/2566043004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:115249: +<histogram_suffixes name="RendererMemoryTiming"> Optional nit: It's fine to use separator="." here too, if you want to keep dots throughout the name rather than mixing them with underscores. https://codereview.chromium.org/2566043004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:115250: + <suffix name="DidFinishLoad" label="DidFinishLoad for all frames"/> nit: I'd phrase this label as something like "Recorded each time DidFinishLoad is called for *any* frame within the page."
https://codereview.chromium.org/2566043004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2566043004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:115231: + <suffix name="BlinkGC" label="Constrained to the BlinkGC allocator"/> On 2017/01/12 02:17:24, Ilya Sherman wrote: > nit: Please add base="true" for all of these suffixes as well. base="true" just > means there's no histogram named "Memory.Experimental.Renderer.BlinkGC" -- it's > used a base name that generates actual histogram names via suffixes. Done. https://codereview.chromium.org/2566043004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:115249: +<histogram_suffixes name="RendererMemoryTiming"> On 2017/01/12 02:17:24, Ilya Sherman wrote: > Optional nit: It's fine to use separator="." here too, if you want to keep dots > throughout the name rather than mixing them with underscores. Done. https://codereview.chromium.org/2566043004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:115250: + <suffix name="DidFinishLoad" label="DidFinishLoad for all frames"/> On 2017/01/12 02:17:24, Ilya Sherman wrote: > nit: I'd phrase this label as something like "Recorded each time DidFinishLoad > is called for *any* frame within the page." Done.
The CQ bit was checked by keishi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, primiano@chromium.org, jam@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2566043004/#ps120001 (title: "Fixed nits")
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by keishi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, primiano@chromium.org, jam@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2566043004/#ps140001 (title: "Fixed test failure")
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": 1484209341809070,
"parent_rev": "80aedca1fd5f6a735391ee96f7dbe24220d05782", "commit_rev":
"51ed0d54153a164e68155447fcd1cfcb393f1870"}
Message was sent while issue was closed.
Description was changed from ========== Add renderer memory metrics We will collect per allocator memory usage metrics for the renderer at DidFinishLoad and Shutdown. GetRendererMemoryMetrics generalizes the memory info collection that was used in RecordPurgeAndSuspendMetrics for use with my new metrics. GetRendererMemoryMetrics is in RenderThreadImpl because it accesses discardable_shared_memory_manager_. BUG= ========== to ========== Add renderer memory metrics We will collect per allocator memory usage metrics for the renderer at DidFinishLoad and Shutdown. GetRendererMemoryMetrics generalizes the memory info collection that was used in RecordPurgeAndSuspendMetrics for use with my new metrics. GetRendererMemoryMetrics is in RenderThreadImpl because it accesses discardable_shared_memory_manager_. BUG= Review-Url: https://codereview.chromium.org/2566043004 Cr-Commit-Position: refs/heads/master@{#443195} Committed: https://chromium.googlesource.com/chromium/src/+/51ed0d54153a164e68155447fcd1... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/51ed0d54153a164e68155447fcd1... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
