Issue metadata
Sign in to add a comment
|
Don't call process_metrics->GetMemoryBytes in OSMetrics::FillOSMemoryDump on macOS |
||||||||||||||||||||||
Issue descriptionThe 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.
,
Sep 11 2017
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
,
Sep 11 2017
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
,
Sep 11 2017
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
,
Sep 12 2017
,
Sep 18 2017
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?
,
Sep 18 2017
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. :)
,
Sep 19 2017
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.
,
Sep 19 2017
,
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 |
|||||||||||||||||||||||
Comment 1 by hjd@chromium.org
, Aug 31 2017