Reduce overhead of task manager memory footprint computation |
|||||
Issue descriptionAll we need is a single number, retrieved from a single syscall. No point in making expensive computations.
,
Jun 11 2018
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?
,
Jun 11 2018
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?
,
Jun 11 2018
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.
,
Jun 12 2018
lalitm said that he was working on a fix.
,
Jun 12 2018
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.
,
Jun 13 2018
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].
,
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
,
Jun 14 2018
This should be fixed in the next canary.
,
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
,
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
,
Jun 18 2018
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.
,
Jun 26 2018
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.
,
Jun 26 2018
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.
,
Jul 9
Issue 860603 has been merged into this issue. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by brucedaw...@chromium.org
, Jun 11 2018180 KB
180 KB View Download