New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 851712 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Reduce overhead of task manager memory footprint computation

Project Member Reported by erikc...@chromium.org, Jun 11 2018

Issue description

All we need is a single number, retrieved from a single syscall. No point in making expensive computations.
 
TL;DR - the task manager column in Chrome's task manager is more expensive than necessary due to calculating of a full memory graph when just a single number is needed.

I've attached a screenshot of a flame graph of an ETW trace that sort of summarizes the overhead. I've filtered out the irrelevant stuff to make it clearer. The flame graph represents ~1300 ms of execution over ~18 s, so a bit less than 10% of a core, with ~20 renderer processes. I can share the trace if you want but I suspect this is more than adequate.

The overhead is presumed to be about 0.5% of a core per renderer process, although this estimate should be taken with a large grain of salt. With many tabs open this is enough to make the task manager less useful by making it the main consumer of CPU time whenever it is open.

MemoryOverhead_bug851712.PNG
180 KB View Download
Cc: lalitm@chromium.org brucedaw...@chromium.org
My initial thought was that we were fetching and computing unnecessary information [e.g. by requesting all the allocator dumps]. But looking at the logic in https://cs.chromium.org/chromium/src/chrome/browser/task_manager/sampling/task_manager_impl.cc?l=588, we're actually not requesting any allocator_dumps at all. So this is the overhead of the graph computation and on empty (?) graph?

What is the overhead on a non-empty graph?
Uh really? I thought we had a bug somewhere about the fact that we request all allocator dumps even if we want just the syscall.
Can somebody check (just add a printf to MemoryDumpManager::InvokeOnMemoryDump) if we are invoking all the MDPs (I would bet so, if this is perf visible) or if this is about computations on an empty graph (I would be quite surprised).

In perfetto we manage to read, parse and protoify ~1000s sched events per second with 1-2% cpu, would be quite surprised if it took 10% to iterate over a size=1 graph. But the world is full of surprises, so maybe we are doing something wrong there. 

Bruce, do you have a non-PNG version of that flamechart, to see which code paths are we actually hitting?
The actual trace can be found here:

https://drive.google.com/file/d/1sCOFyImyIX8AVvDXXDUtCw4Su-fLcMbR/view?usp=sharing

Ignore all of the graphs except for CPU Usage (Sampled) - in fact you can/should close all of the other ones. Make sure you add Chrome's symbol server (Trace-> Configure Symbol Paths) and load symbols (Trace-> Load Symbols) and be aware that loading symbols the first time takes quite a while.

Cc: erikc...@chromium.org
Owner: lalitm@chromium.org
lalitm said that he was working on a fix.
It looks like the easiest way to see that this is happening is to open lots of tabs, preferably ones that don't do much, have about:blank visible (to minimize background activity) and then open Chrome's task manager. Watch the CPU usage of the browser process for a while. Then add/remove the Memory Footprint column and see the change.

There is a lot of noise in this measurement but on my work desktop machine the difference is quite obvious even with the noise. On my home laptop the difference was less clear, I'm not sure why.

So, Chrome's task manager can be used to show that the Memory Footprint column expensive, ETW profiling (or VS profiling) can be used to see where the cost is coming from.

Owner: erikc...@chromium.org
I went through every consumer of MemoryInstrumentation to figure out the current use cases.

BackgroundMemoryTracingObserver [sid's light heap reports]
  RequestGlobalDumpAndAppendToTrace(EXPLICIT, BACKGROUND)

TracingHandler::RequestMemoryDump [devtools tracing protocol]
  RequestGlobalDumpAndAppendToTrace(EXPLICIT, DETAILED)

heap_profiling::Supervisor [heap profiling]
  RequestGlobalDumpAndAppendToTrace(EXPLICIT, BACKGROUND)

ProcessMemoryMetricsEmitter [UMA + UKMs]
  RequestGlobalDumpForPid(non-empty mad_list)

The following classes: TaskManager, DataReductionProxyMetricsObserver, PrerenderContents, MemoryDetails, BackgroundProfilingTriggers, RenderProcessProbeImpl
are all the same. They pass an empty mad_list to RequestGlobalDump*, and just need private footprint.

PERIODIC_INTERVAL is used by chrome://tracing and is consumed directly by MemoryDumpManager.
MemoryDumpLevelOfDetail::LIGHT appears to be unused [I guess it can be triggered directly by custom trace configs].

Here's a proposal for a change to the public API of MemoryInstrumentation:
RequestGlobalDumpWithFiltering [used by ProcessMemoryMetricsEmitter, takes a mad_list, same behavior as before]
RequestGlobalDumpAndAppendToTrace [used by first 3 consumers above, no change]
RequestPrivateMemoryFootprint [used by all remaining consumers].
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/43e035ee01ec29dad3e21db131d51be10cac2472

commit 43e035ee01ec29dad3e21db131d51be10cac2472
Author: erikchen <erikchen@chromium.org>
Date: Thu Jun 14 21:18:51 2018

Add RequestPrivateMemoryFootprint method to MemoryInstrumentation.

Most clients only need private memory footprint, but the existing methods would
still query all the MemoryDumpProviders and then filter the results. This CL
adds a new method that avoids querying MemoryDumpProviders.

Bug:  851712 
Change-Id: I269be2b5a74c32205ba6f4087a9bb8affb9cffb6
Reviewed-on: https://chromium-review.googlesource.com/1098623
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567417}
[modify] https://crrev.com/43e035ee01ec29dad3e21db131d51be10cac2472/chrome/browser/memory_details.cc
[modify] https://crrev.com/43e035ee01ec29dad3e21db131d51be10cac2472/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
[modify] https://crrev.com/43e035ee01ec29dad3e21db131d51be10cac2472/chrome/browser/prerender/prerender_contents.cc
[modify] https://crrev.com/43e035ee01ec29dad3e21db131d51be10cac2472/chrome/browser/profiling_host/background_profiling_triggers.cc
[modify] https://crrev.com/43e035ee01ec29dad3e21db131d51be10cac2472/chrome/browser/resource_coordinator/render_process_probe.cc
[modify] https://crrev.com/43e035ee01ec29dad3e21db131d51be10cac2472/chrome/browser/task_manager/sampling/task_manager_impl.cc
[modify] https://crrev.com/43e035ee01ec29dad3e21db131d51be10cac2472/content/browser/tracing/memory_instrumentation_browsertest.cc
[modify] https://crrev.com/43e035ee01ec29dad3e21db131d51be10cac2472/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc
[modify] https://crrev.com/43e035ee01ec29dad3e21db131d51be10cac2472/services/resource_coordinator/memory_instrumentation/coordinator_impl.h
[modify] https://crrev.com/43e035ee01ec29dad3e21db131d51be10cac2472/services/resource_coordinator/memory_instrumentation/queued_request.cc
[modify] https://crrev.com/43e035ee01ec29dad3e21db131d51be10cac2472/services/resource_coordinator/memory_instrumentation/queued_request.h
[modify] https://crrev.com/43e035ee01ec29dad3e21db131d51be10cac2472/services/resource_coordinator/memory_instrumentation/queued_request_dispatcher.cc
[modify] https://crrev.com/43e035ee01ec29dad3e21db131d51be10cac2472/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.cc
[modify] https://crrev.com/43e035ee01ec29dad3e21db131d51be10cac2472/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h
[modify] https://crrev.com/43e035ee01ec29dad3e21db131d51be10cac2472/services/resource_coordinator/public/cpp/memory_instrumentation/tracing_integration_unittest.cc
[modify] https://crrev.com/43e035ee01ec29dad3e21db131d51be10cac2472/services/resource_coordinator/public/mojom/memory_instrumentation/memory_instrumentation.mojom

Status: Fixed (was: Assigned)
This should be fixed in the next canary.
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/99e5894e6777aa11e45dc5811839921658eda878

commit 99e5894e6777aa11e45dc5811839921658eda878
Author: Robert Liao <robliao@chromium.org>
Date: Fri Jun 15 14:32:38 2018

Revert "Add RequestPrivateMemoryFootprint method to MemoryInstrumentation."

This reverts commit 43e035ee01ec29dad3e21db131d51be10cac2472.

Reason for revert: browser_test failures on
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win7%20Tests%20%28dbg%29%281%29 starting around 69747

Small subset of tests failing:
PrerenderBrowserTest.PrerenderPageNewTab
DefaultIsolation/TaskManagerOOPIFBrowserTest.OrderingOfDependentRows/0
PrerenderBrowserTest.PrerenderDownloadClientRedirect
PrintPreviewBrowserTest.TaskManagerNewPrintPreview
TabManagerTest.OomPressureListener

Stack:
Received fatal exception EXCEPTION_BREAKPOINT
Backtrace:
base::win::RegisterInvalidParamHandler [0x738B4167+71]
invalid_parameter [0x6FDAECD1+161]
std::_Tree_const_iterator<std::_Tree_val<std::_Tree_simple_types<std::pair<unsigned long const ,std::unique_ptr<memory_instrumentation::GlobalDumpGraph::Process,std::default_delete<memory_instrumentation::GlobalDumpGraph::Process> > > > > >::operator* [0x6296CB0C+156]
std::_Tree_const_iterator<std::_Tree_val<std::_Tree_simple_types<std::pair<unsigned long const ,std::unique_ptr<memory_instrumentation::GlobalDumpGraph::Process,std::default_delete<memory_instrumentation::GlobalDumpGraph::Process> > > > > >::operator-> [0x6299A3C1+17]
memory_instrumentation::QueuedRequestDispatcher::Finalize [0x62998EF4+3364]
memory_instrumentation::CoordinatorImpl::FinalizeGlobalMemoryDumpIfAllManagersReplied [0x629303EB+587]
memory_instrumentation::CoordinatorImpl::OnOSMemoryDumpResponse [0x629324F3+819]

Original change's description:
> Add RequestPrivateMemoryFootprint method to MemoryInstrumentation.
> 
> Most clients only need private memory footprint, but the existing methods would
> still query all the MemoryDumpProviders and then filter the results. This CL
> adds a new method that avoids querying MemoryDumpProviders.
> 
> Bug:  851712 
> Change-Id: I269be2b5a74c32205ba6f4087a9bb8affb9cffb6
> Reviewed-on: https://chromium-review.googlesource.com/1098623
> Reviewed-by: Primiano Tucci <primiano@chromium.org>
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Commit-Queue: Erik Chen <erikchen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#567417}

TBR=dcheng@chromium.org,thakis@chromium.org,primiano@chromium.org,erikchen@chromium.org,lalitm@chromium.org

Change-Id: I5dd27418aad49f2fcfbcb10c5510f11b91f4d875
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  851712 
Reviewed-on: https://chromium-review.googlesource.com/1102518
Reviewed-by: Robert Liao <robliao@chromium.org>
Commit-Queue: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567641}
[modify] https://crrev.com/99e5894e6777aa11e45dc5811839921658eda878/chrome/browser/memory_details.cc
[modify] https://crrev.com/99e5894e6777aa11e45dc5811839921658eda878/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
[modify] https://crrev.com/99e5894e6777aa11e45dc5811839921658eda878/chrome/browser/prerender/prerender_contents.cc
[modify] https://crrev.com/99e5894e6777aa11e45dc5811839921658eda878/chrome/browser/profiling_host/background_profiling_triggers.cc
[modify] https://crrev.com/99e5894e6777aa11e45dc5811839921658eda878/chrome/browser/resource_coordinator/render_process_probe.cc
[modify] https://crrev.com/99e5894e6777aa11e45dc5811839921658eda878/chrome/browser/task_manager/sampling/task_manager_impl.cc
[modify] https://crrev.com/99e5894e6777aa11e45dc5811839921658eda878/content/browser/tracing/memory_instrumentation_browsertest.cc
[modify] https://crrev.com/99e5894e6777aa11e45dc5811839921658eda878/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc
[modify] https://crrev.com/99e5894e6777aa11e45dc5811839921658eda878/services/resource_coordinator/memory_instrumentation/coordinator_impl.h
[modify] https://crrev.com/99e5894e6777aa11e45dc5811839921658eda878/services/resource_coordinator/memory_instrumentation/queued_request.cc
[modify] https://crrev.com/99e5894e6777aa11e45dc5811839921658eda878/services/resource_coordinator/memory_instrumentation/queued_request.h
[modify] https://crrev.com/99e5894e6777aa11e45dc5811839921658eda878/services/resource_coordinator/memory_instrumentation/queued_request_dispatcher.cc
[modify] https://crrev.com/99e5894e6777aa11e45dc5811839921658eda878/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.cc
[modify] https://crrev.com/99e5894e6777aa11e45dc5811839921658eda878/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h
[modify] https://crrev.com/99e5894e6777aa11e45dc5811839921658eda878/services/resource_coordinator/public/cpp/memory_instrumentation/tracing_integration_unittest.cc
[modify] https://crrev.com/99e5894e6777aa11e45dc5811839921658eda878/services/resource_coordinator/public/mojom/memory_instrumentation/memory_instrumentation.mojom

Project Member

Comment 11 by bugdroid1@chromium.org, Jun 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0c63410712948c3a21f8a7259db82e40c92080c8

commit 0c63410712948c3a21f8a7259db82e40c92080c8
Author: erikchen <erikchen@chromium.org>
Date: Fri Jun 15 22:05:06 2018

[Reland #1] Add RequestPrivateMemoryFootprint method to MemoryInstrumentation.

The original CL stopped requesting chrome dumps for
RequestPrivateMemoryFootprint, but there was a block in
QueuedRequestDispatcher::Finalize that assumed that the chrome dumps did exist.

> Most clients only need private memory footprint, but the existing methods would
> still query all the MemoryDumpProviders and then filter the results. This CL
> adds a new method that avoids querying MemoryDumpProviders.
>
> Bug:  851712 
> Reviewed-on: https://chromium-review.googlesource.com/1098623
> Reviewed-by: Primiano Tucci <primiano@chromium.org>
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Commit-Queue: Erik Chen <erikchen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#567417}

Bug:  851712 
Change-Id: Ibac7c2189ea74f2029458ba3088159259aab5096
TBR: dcheng@chromium.org, thakis@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/1102724
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567808}
[modify] https://crrev.com/0c63410712948c3a21f8a7259db82e40c92080c8/chrome/browser/memory_details.cc
[modify] https://crrev.com/0c63410712948c3a21f8a7259db82e40c92080c8/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
[modify] https://crrev.com/0c63410712948c3a21f8a7259db82e40c92080c8/chrome/browser/prerender/prerender_contents.cc
[modify] https://crrev.com/0c63410712948c3a21f8a7259db82e40c92080c8/chrome/browser/profiling_host/background_profiling_triggers.cc
[modify] https://crrev.com/0c63410712948c3a21f8a7259db82e40c92080c8/chrome/browser/resource_coordinator/render_process_probe.cc
[modify] https://crrev.com/0c63410712948c3a21f8a7259db82e40c92080c8/chrome/browser/task_manager/sampling/task_manager_impl.cc
[modify] https://crrev.com/0c63410712948c3a21f8a7259db82e40c92080c8/content/browser/tracing/memory_instrumentation_browsertest.cc
[modify] https://crrev.com/0c63410712948c3a21f8a7259db82e40c92080c8/services/resource_coordinator/memory_instrumentation/coordinator_impl.cc
[modify] https://crrev.com/0c63410712948c3a21f8a7259db82e40c92080c8/services/resource_coordinator/memory_instrumentation/coordinator_impl.h
[modify] https://crrev.com/0c63410712948c3a21f8a7259db82e40c92080c8/services/resource_coordinator/memory_instrumentation/queued_request.cc
[modify] https://crrev.com/0c63410712948c3a21f8a7259db82e40c92080c8/services/resource_coordinator/memory_instrumentation/queued_request.h
[modify] https://crrev.com/0c63410712948c3a21f8a7259db82e40c92080c8/services/resource_coordinator/memory_instrumentation/queued_request_dispatcher.cc
[modify] https://crrev.com/0c63410712948c3a21f8a7259db82e40c92080c8/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.cc
[modify] https://crrev.com/0c63410712948c3a21f8a7259db82e40c92080c8/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h
[modify] https://crrev.com/0c63410712948c3a21f8a7259db82e40c92080c8/services/resource_coordinator/public/cpp/memory_instrumentation/tracing_integration_unittest.cc
[modify] https://crrev.com/0c63410712948c3a21f8a7259db82e40c92080c8/services/resource_coordinator/public/mojom/memory_instrumentation/memory_instrumentation.mojom

Manual testing with the latest canary build suggests that this is fixed. I can't see any increase in CPU usage when the Memory Footprint column is open.

It's difficult to be completely certain due to the amount of noise in CPU usage but I'm fairly confident that the overhead of the memory footprint is now greatly reduced.

Thanks! This makes Chrome's task manager much more useful.

I just verified that this bug is in M67 as well, and therefore in M68. It's obviously too late for M67 but any thoughts on merging it to M68?

On my M67 browser it currently makes the difference between ~5% and ~15% CPU usage.
I don't think this bug affects enough users with enough severity to justify a merge to M68.

I do tend to be on the conservative side of merging - you're welcome to request a merge from the release manager and I'll do the merge if they agree.
 Issue 860603  has been merged into this issue.

Sign in to add a comment