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

Issue 607533 link

Starred by 2 users
Status: Assigned
Owner:
ssid@chromium.org
Cc:
█ picksi@chromium.org
█ tasak@chromium.org
ssid@chromium.org
█ mariakho...@chromium.org
█ nduca@chromium.org
haraken@chromium.org
█ dskiba@chromium.org
primiano@chromium.org
bashi@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 712767



Sign in to add a comment

MemoryInfra: trigger dumps on peak

Project Member Reported by primiano@chromium.org, Apr 28 2016 
Background context:go/memory-infra: memory profiling in chrome://tracing

This emerged during trim convergence. We should have a mode in memory-infra where dumps are triggered on a peak.
The idea is to do something like this:
After each messageloop task ends, cheaply measure the RSS and if it increased by X%, trigger a dump. This will be useful for benchmarking peak memory usage.

Comment 1 by ericrk@chromium.org, May 4 2016 

For things like GPU, where memory may not be completely covered by RSS, we may want an additional way to poll things? It may be enough to have a global GPU memory estimator (which does nothing except in the GPU process, where it returns our cached value), and use this as well as RSS?

Comment 2 by primiano@chromium.org, May 5 2016 

Yes I was thinking something along the lines of adding an optional virtual method to the MemoryDumpProvider interface, something along the lines of:

class MemoryDumpProvider {
  .. the usual OnMemoryDump ...
  size_t GetFastTotal();
  size_t GetFastTotalThreshold();
}

so that GPU can provide its own total and threshold for triggering (or eventually could do the math itself?)

Comment 3 by petrcermak@chromium.org, Jul 1 2016

Owner: primiano@chromium.org
Any updates on this?

Comment 4 by primiano@chromium.org, Jul 1 2016

Cc: ssid@chromium.org
Owner: ssid@chromium.org
I think ssid@ was looking into this for bulk reports?
Please reassign to me if that's not the case.

Comment 5 by petrcermak@chromium.org, Oct 5 2016

Cc: -petrcermak@chromium.org

Comment 6 by petrcermak@chromium.org, Oct 5 2016

Cc: -petrcermak@chromium.org
Project Member

Comment 8 by bugdroid1@chromium.org, Dec 19 2016 

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

commit 4460b9b6393448adc011207ae63380a70ddba369
Author: ssid <ssid@chromium.org>
Date: Mon Dec 19 00:52:11 2016

[memory-infra] Add support for polling memory totals in MemoryDumpManager

This adds new method in MemoryDumpProvider interface PollFastMemoryTotal
which is used for polling memory totals in process. The dump provider
has to enable this feature while registration.
For Simplicity of polling logic the dump providers supporting polling
must not have task runner affinity. Polling will be done on dump thread.

BUG=607533

Review-Url: https://codereview.chromium.org/2537363003
Cr-Commit-Position: refs/heads/master@{#439390}

[modify] https://crrev.com/4460b9b6393448adc011207ae63380a70ddba369/base/trace_event/memory_dump_manager.cc
[modify] https://crrev.com/4460b9b6393448adc011207ae63380a70ddba369/base/trace_event/memory_dump_manager.h
[modify] https://crrev.com/4460b9b6393448adc011207ae63380a70ddba369/base/trace_event/memory_dump_manager_unittest.cc
[modify] https://crrev.com/4460b9b6393448adc011207ae63380a70ddba369/base/trace_event/memory_dump_provider.h
[modify] https://crrev.com/4460b9b6393448adc011207ae63380a70ddba369/base/trace_event/memory_dump_session_state.cc
[modify] https://crrev.com/4460b9b6393448adc011207ae63380a70ddba369/base/trace_event/memory_dump_session_state.h
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 19 2016 

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

commit 3583cfe73e8f0ac811ad2e978fcd1cfaebce6ba6
Author: ssid <ssid@chromium.org>
Date: Mon Dec 19 04:11:12 2016

[memory-infra] Implement PollFastMemoryTotal in ProcessMetricsMemoryDumpProvider.

For fast polling the dump provider keeps the /proc/<pid>/statm files
open during the tracing session.

BUG=607533

Review-Url: https://codereview.chromium.org/2568313004
Cr-Commit-Position: refs/heads/master@{#439405}

[modify] https://crrev.com/3583cfe73e8f0ac811ad2e978fcd1cfaebce6ba6/components/tracing/common/process_metrics_memory_dump_provider.cc
[modify] https://crrev.com/3583cfe73e8f0ac811ad2e978fcd1cfaebce6ba6/components/tracing/common/process_metrics_memory_dump_provider.h
[modify] https://crrev.com/3583cfe73e8f0ac811ad2e978fcd1cfaebce6ba6/components/tracing/common/process_metrics_memory_dump_provider_unittest.cc
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 21 2016 

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

commit cc4d7737106490e79b40e4bc946405f324c0d834
Author: ssid <ssid@chromium.org>
Date: Wed Dec 21 22:22:14 2016

Make memory-infra tests register / unregister with thread safety

The DCHECK for correct thread at unregistration fires only when tracing
is enabled. As a first step to make this check stricter, fix the tests
to unregister on right thread.
Move the DCHECK that tests for task runner for polling MDP to
registration time.

BUG=643438,607533

Review-Url: https://codereview.chromium.org/2592543002
Cr-Commit-Position: refs/heads/master@{#440239}

[modify] https://crrev.com/cc4d7737106490e79b40e4bc946405f324c0d834/base/trace_event/memory_dump_manager.cc
[modify] https://crrev.com/cc4d7737106490e79b40e4bc946405f324c0d834/base/trace_event/memory_dump_manager_unittest.cc
[modify] https://crrev.com/cc4d7737106490e79b40e4bc946405f324c0d834/components/tracing/child/child_trace_message_filter_browsertest.cc
[modify] https://crrev.com/cc4d7737106490e79b40e4bc946405f324c0d834/content/browser/tracing/memory_tracing_browsertest.cc

Comment 11 by primiano@chromium.org, Jan 4 2017

Components: Internals>Instrumentation>Memory
Project Member

Comment 12 by bugdroid1@chromium.org, Feb 17 2017 

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

commit 5047017bdca8641764919bcbb25b60b8d89c5684
Author: ssid <ssid@chromium.org>
Date: Fri Feb 17 22:26:15 2017

[tracing] Implement polling in MemoryDumpManager

Implement polling for memory totals when peak triggers are given in
trace config. It also triggers dumps if there is a big increase in
memory total.

BUG=607533

Review-Url: https://codereview.chromium.org/2582453002
Cr-Commit-Position: refs/heads/master@{#451386}

[modify] https://crrev.com/5047017bdca8641764919bcbb25b60b8d89c5684/base/BUILD.gn
[modify] https://crrev.com/5047017bdca8641764919bcbb25b60b8d89c5684/base/trace_event/heap_profiler_heap_dump_writer.cc
[modify] https://crrev.com/5047017bdca8641764919bcbb25b60b8d89c5684/base/trace_event/memory_dump_manager.cc
[modify] https://crrev.com/5047017bdca8641764919bcbb25b60b8d89c5684/base/trace_event/memory_dump_manager.h
[modify] https://crrev.com/5047017bdca8641764919bcbb25b60b8d89c5684/base/trace_event/memory_dump_manager_unittest.cc
[add] https://crrev.com/5047017bdca8641764919bcbb25b60b8d89c5684/base/trace_event/memory_dump_scheduler.cc
[add] https://crrev.com/5047017bdca8641764919bcbb25b60b8d89c5684/base/trace_event/memory_dump_scheduler.h
[modify] https://crrev.com/5047017bdca8641764919bcbb25b60b8d89c5684/base/trace_event/memory_dump_session_state.cc
[modify] https://crrev.com/5047017bdca8641764919bcbb25b60b8d89c5684/base/trace_event/memory_dump_session_state.h
[modify] https://crrev.com/5047017bdca8641764919bcbb25b60b8d89c5684/tools/gn/bootstrap/bootstrap.py

Comment 13 by ssid@chromium.org, Mar 8 2017

Cc: primiano@chromium.org mariakho...@chromium.org dskiba@chromium.org
 Issue 609935  has been merged into this issue.
Project Member

Comment 14 by bugdroid1@chromium.org, Mar 10 2017 

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

commit 203654f1cb443b997c58dfda0dbb8e11d7222f97
Author: ssid <ssid@chromium.org>
Date: Fri Mar 10 21:13:56 2017

[memory-infra] Implement peak detection logic

The CL implements peak detection in memory dump scheduler.
More discussion at https://goo.gl/0kOU4A.

BUG=607533

Review-Url: https://codereview.chromium.org/2737153002
Cr-Commit-Position: refs/heads/master@{#456163}

[modify] https://crrev.com/203654f1cb443b997c58dfda0dbb8e11d7222f97/base/trace_event/memory_dump_scheduler.cc
[modify] https://crrev.com/203654f1cb443b997c58dfda0dbb8e11d7222f97/base/trace_event/memory_dump_scheduler.h
Project Member

Comment 15 by bugdroid1@chromium.org, Mar 11 2017 

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

commit 5a15b1b1fa8f202b16250541522d22a070b03e1f
Author: ssid <ssid@chromium.org>
Date: Sat Mar 11 03:07:37 2017

[memory-infra] MemoryDumpScheduler follows config for time between dumps

The scheduler should reset the time since the last dump on all processes
when every global dump happens. So, MemoryDumpManager should notify the
scheduler when dumps are triggered.
Use enum instead of multiple bool to store the state of polling.

BUG=607533

Review-Url: https://codereview.chromium.org/2736283003
Cr-Commit-Position: refs/heads/master@{#456268}

[modify] https://crrev.com/5a15b1b1fa8f202b16250541522d22a070b03e1f/base/trace_event/memory_dump_manager.cc
[modify] https://crrev.com/5a15b1b1fa8f202b16250541522d22a070b03e1f/base/trace_event/memory_dump_scheduler.cc
[modify] https://crrev.com/5a15b1b1fa8f202b16250541522d22a070b03e1f/base/trace_event/memory_dump_scheduler.h
[modify] https://crrev.com/5a15b1b1fa8f202b16250541522d22a070b03e1f/services/resource_coordinator/memory/coordinator/coordinator_impl.cc
Project Member

Comment 18 by bugdroid1@chromium.org, Mar 23 2017 

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

commit 82b21b9b3985d1795b8198e52580ee188b89bebc
Author: ssid <ssid@chromium.org>
Date: Thu Mar 23 00:30:04 2017

Revert of Revert "[memory-infra] Add unittests for peak detection and NotifyDumpTriggered" (patchset #1 id:1 of https://codereview.chromium.org/2766933004/ )

Reason for revert:
Reland with extra asserts to debug the failure in waterfall.

Original issue's description:
> Revert "[memory-infra] Add unittests for peak detection and NotifyDumpTriggered"
>
> Reason: breaks CFI buildbots
>
> https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/7700
> https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux%20ToT/builds/5852
>
> This reverts the CL https://codereview.chromium.org/2743663005
>
> BUG=607533
> TBR=dcheng
>
> Review-Url: https://codereview.chromium.org/2766933004
> Cr-Commit-Position: refs/heads/master@{#458787}
> Committed: https://chromium.googlesource.com/chromium/src/+/68d51c6dd7715dcb357341845d7f8103b82febb1

TBR=primiano@chromium.org,dcheng@chromium.org,krasin@chromium.org
BUG=607533

Review-Url: https://codereview.chromium.org/2766303002
Cr-Commit-Position: refs/heads/master@{#458953}

[modify] https://crrev.com/82b21b9b3985d1795b8198e52580ee188b89bebc/base/BUILD.gn
[modify] https://crrev.com/82b21b9b3985d1795b8198e52580ee188b89bebc/base/trace_event/memory_dump_scheduler.h
[add] https://crrev.com/82b21b9b3985d1795b8198e52580ee188b89bebc/base/trace_event/memory_dump_scheduler_unittest.cc

Comment 21 by hjd@chromium.org, Apr 11 2017

Labels: Hotlist-MemoryInfraUma
Project Member

Comment 22 by bugdroid1@chromium.org, Apr 11 2017 

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

commit 3821fa74fa559fbaabda7c9ca82ffa244c2e45a7
Author: primiano <primiano@chromium.org>
Date: Tue Apr 11 15:50:36 2017

memory-infra: port peak detection logic to MemoryPeakDetector

This CL ports the memory peak detection of MemoryDumpScheduler
into the new, self-contained, MemoryPeakDetector.
This CL has no functional effects, since the new peak detector is not
used yet.

BUG=607533

Review-Url: https://codereview.chromium.org/2793023002
Cr-Commit-Position: refs/heads/master@{#463642}

[modify] https://crrev.com/3821fa74fa559fbaabda7c9ca82ffa244c2e45a7/base/trace_event/memory_peak_detector.cc
[modify] https://crrev.com/3821fa74fa559fbaabda7c9ca82ffa244c2e45a7/base/trace_event/memory_peak_detector.h
[modify] https://crrev.com/3821fa74fa559fbaabda7c9ca82ffa244c2e45a7/base/trace_event/memory_peak_detector_unittest.cc
Project Member

Comment 23 by bugdroid1@chromium.org, Apr 12 2017 

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

commit 57cee813cd7d1c400275a5cb8e95612a4fae0fd4
Author: primiano <primiano@chromium.org>
Date: Wed Apr 12 11:15:32 2017

memory-infra: Switch to MemoryPeakDetector and simplify MemoryDumpScheduler

This CL concludes the refactoring of the old monolithic MemoryDumpScheduler in
the new MemoryPeakDetector (for peaks) + MemoryDumpScheduler (for periodic
dumps only). This refactoring was mostly done to clean up the entanglement of
threading dependencies between MemoryDumpManager and the scheduler.

Note for perf sheriffs:
----------------------
If memory metrics are skewed, or system_health memory benchmarks start
failing this is very likely the culprit. Proceed with revert in this case.

BUG=607533

Review-Url: https://codereview.chromium.org/2799023002
Cr-Commit-Position: refs/heads/master@{#463991}

[modify] https://crrev.com/57cee813cd7d1c400275a5cb8e95612a4fae0fd4/base/trace_event/memory_dump_manager.cc
[modify] https://crrev.com/57cee813cd7d1c400275a5cb8e95612a4fae0fd4/base/trace_event/memory_dump_manager.h
[modify] https://crrev.com/57cee813cd7d1c400275a5cb8e95612a4fae0fd4/base/trace_event/memory_dump_manager_unittest.cc
[modify] https://crrev.com/57cee813cd7d1c400275a5cb8e95612a4fae0fd4/base/trace_event/memory_dump_scheduler.cc
[modify] https://crrev.com/57cee813cd7d1c400275a5cb8e95612a4fae0fd4/base/trace_event/memory_dump_scheduler.h
[modify] https://crrev.com/57cee813cd7d1c400275a5cb8e95612a4fae0fd4/base/trace_event/memory_dump_scheduler_unittest.cc
[modify] https://crrev.com/57cee813cd7d1c400275a5cb8e95612a4fae0fd4/base/trace_event/memory_peak_detector.cc
[modify] https://crrev.com/57cee813cd7d1c400275a5cb8e95612a4fae0fd4/base/trace_event/memory_peak_detector.h
[modify] https://crrev.com/57cee813cd7d1c400275a5cb8e95612a4fae0fd4/base/trace_event/memory_peak_detector_unittest.cc

Comment 24 by dskiba@chromium.org, Apr 12 2017 

Something above changed how startup tracing works: it started to create two details dumps (one at zero, another after a specified delay).

Was that intentional?

Comment 25 by ssid@chromium.org, Apr 12 2017 

Re #24: Yes primiano's change which moved periodic dumps from using repeating timer to a PostDelayedTask() logic also changed the first dump to be triggered at t=0 instead of t=period.

Comment 26 by primiano@chromium.org, Apr 13 2017 

Ah yeah I mostly did thinking to TraceOnTap. Is it causing any problem?

Comment 27 by dskiba@chromium.org, Apr 13 2017 

I would prefer startup tracing to have just a single dump, it's more straightforward (i.e. you don't have to think about which one is the right one).

Comment 28 by primiano@chromium.org, Apr 13 2017 

Yup ssid explained me the problem about startup tracing and filed a cl to fix it. SGTM.
(it was https://codereview.chromium.org/2815963002/ which then got closed)
Project Member

Comment 29 by bugdroid1@chromium.org, Apr 13 2017 

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

commit 1a345de25847354fb1e1b1b81bbd00a64af8d55b
Author: ssid <ssid@chromium.org>
Date: Thu Apr 13 20:52:02 2017

[memory-infra] Trigger first periodic dump after initial period

The periodic dumps behavior was to create memory dump after the first
period ended. It is makes startup tracing worse in case we want a
single dump and not the dump at start. So, get back old behavior.

BUG=607533
R=primiano@chromium.org

Review-Url: https://codereview.chromium.org/2815963002
Cr-Commit-Position: refs/heads/master@{#464539}

[modify] https://crrev.com/1a345de25847354fb1e1b1b81bbd00a64af8d55b/base/trace_event/memory_dump_scheduler.cc

Comment 30 by primiano@chromium.org, May 5 2017

Blocking: 712767

Comment 31 by benhenry@chromium.org, Aug 1

Status: Assigned (was: Untriaged)
►

Sign in to add a comment