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

Issue 760777 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug
Hotlist-MemoryInfra

Blocking:
issue 756276



Sign in to add a comment

Don't call process_metrics->GetMemoryBytes in OSMetrics::FillOSMemoryDump on macOS

Project Member Reported by erikc...@chromium.org, Aug 30 2017

Issue description

The call iterates through all vm regions. This is a little-bit slow. It looks like the only point of this metric is for tracing:

https://cs.chromium.org/chromium/src/services/resource_coordinator/public/cpp/memory_instrumentation/tracing_observer.cc?type=cs&q=resident_set_kb+file:resource_coordinator&sq=package:chromium&l=34

So we shouldn't call it for non-tracing use-caes.
 

Comment 1 by hjd@chromium.org, Aug 31 2017

Status: Started (was: Assigned)
Blocking: 756276
Was discussing with +erikchen, this seems a blocker for  Issue 756276  (task manager).
hjd@ you came up with a very nice table that explains what is needed where, can you post it here?
If my understanding is correct, we want the vmmaps only for the level_of_detail=DETAILED dumps (Which in turn is what is used by tacing), but not the others
copy-pasting from internal chat:

TL;DR
it seems that the only non-tracing related case today why we end up calling process_metrics->GetMemoryBytes is for "Memory.Experimental.Browser2.Resident"
so the options are:
1) is there a faster way to compute that?
2) how much do we case about .Resident UMA/UKM?


Longer version:
- we don't deliberately grab the mmaps (As we do for tracing) but it's a side effect of calling process_metrics->GetMemoryBytes
- This is just to compute |resident_bytes|, which then goes into |RawOSMemDump.resident_set_kb|
- |resident_set_kb| is NOT used for CalculatePrivateFootprintKb(), but is used for the overall CreatePublicOSDump()
- so next question is: who consumes that |resident_set_kb|?  -> Memory.Experimental.Browser2.Resident & friends
Cc: erikc...@chromium.org ssid@chromium.org
Let's just drop ".Resident" UMA/UKM (% first giving a heads up to chrome-memory and uma-team).
AFAICT we had that only for the initial period to compare with the old umas
Labels: -Pri-3 Pri-1
Primiano: I seem to recall a discussion with you where you mentioned that we couldn't drop .Resident...but the details elude me and I can't find the conversation. Could you repost details here?
I intend to expose PrivateMemoryFootprint [via memory_instrumentation interface] to Task Manager. But right now that also does a full vmmap on most platforms to calculate resident memory, on every process, every refresh interval. This itself takes a *huge* amount of computation, and I'd like to avoid that if possible. :)


Owner: erikc...@chromium.org
Re #6:
my rational for not dropping .Resident right now is that shared memory (and hence the full CMM) is not there.
On Android/Linux .Resident today counts also shared memory (but not swapped). If we drop it, we reduce to zero the metrics we have to spot any regressions that happen in the wild in shared memory before full CMM comes in. 

> intend to expose PrivateMemoryFootprint....This itself takes a *huge* amount of computation, and I'd like to avoid that if possible. :)

MemoryInstrumentation::RequestGlobalDump() under the hoods does already a MemoryDumpType::SUMMARY_ONLY, MemoryDumpLevelOfDetail::BACKGROUND dump.

Such dump is designed to be cheap and if we do any expensive operation there that's not good, regardless of the TaskManager (i.e. its bad even for UMA).

The only reason why we do that today is the aforementioned .Resident.

Proposal
--------
Remove the call to process_metrics->GetMemoryBytes() from OSMetrics::FillOSMemoryDump in memory_instrumentation/os_metrics_mac.cc.
That, on mac, will zero the |resident_set_kb| returned by CreatePublicOSDump.
In turn will zero the .Resident UMA in process_memory_metrics_emitter.cc for mac.

Rational: My understanding is that on mac .Resident gives very little benefit and has too high cost.
The cost instead seems minimal on other platforms, and right now gives some better coverage that we don't have yet.
 

Cc: hjd@chromium.org
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 27 2017

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

commit a73ea2bb7c16b3eea245cf396d773bf974ec6ff5
Author: Erik Chen <erikchen@chromium.org>
Date: Wed Sep 27 02:10:23 2017

Don't compute resident memory for memory_instrumentation on macOS.

Computing resident memory requires iterating through all virtual memory pages,
which is slow. The marginal benefit of having resident memory is low, since we
are transitioning over to private memory footprint, which includes both resident
and swapped memory.

Bug: 760777
Change-Id: I8cd5458b52dc3445eb717cc8e4cf4e043d3b9caa
TBR: primiano@chromium.org, asvitkine@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/680617
Reviewed-by: Erik Chen <erikchen@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504548}
[modify] https://crrev.com/a73ea2bb7c16b3eea245cf396d773bf974ec6ff5/chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc
[modify] https://crrev.com/a73ea2bb7c16b3eea245cf396d773bf974ec6ff5/services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_mac.cc

Sign in to add a comment