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

Issue 815328 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

Proposal to remove "total resident", "private dirty", "swapped" columns from analysis view.

Project Member Reported by erikc...@chromium.org, Feb 23 2018

Issue description

Hi all. These columns seem out-dated now that we have private memory footprint.

On macOS, total resident is slightly negative, "private dirty" is meaningless, since it primarily measures shared, resident system libraries, and "swapped" is part of "private memory footprint".

Are these columns more useful on other platforms? Unless they actually help developers solve problems, I'd like to propose getting rid of them, since more columns = more confusion. 
 
Screen Shot 2018-02-23 at 5.57.34 PM.png
126 KB View Download
-total resident:  removal LGTM (even from the UI itself) too misleading and inactionable.
-swapped: removal still okay, although doesn't IMHO hurt on Linux (is it always 0 on mac?). as long as we can track that into the mmaps table that shows when you click on the blue column we are good.

-"private dirty": hmm I'm on the fence here. people in the linux/android world are still too attached to that, let's keep it or we'll have to spend our life answering questions.

I think the real issue is that on mac private_dirty has been misused. It seems to report numbers that are >> private footprint, I think because it report the executable image as "private dirty". So I'd be definitely up for removing that.
Maybe we can just stop emitting from the c++ code on MacOS? What does it report these days? Is it wired up somewhere?

The ideal situation would be having them hiden by default and a "..." icon to expand them. But that's not an option AFAIK in TraceViewer UI and not worth adding it right now.

Great. I'd also like to remove "peak total resident", since we're removing "total resident". ssid@, dskiba@, is that okay?

Comment 3 by ssid@chromium.org, Mar 1 2018

I would like to have the private_dirty and swapped in the Overview section. It is easier to see the breakdown rather than just private footprint. Sometimes we work on reducing swap rather than private footprint since that is what is slowing us down.
I agree we should remove total resident from Overview section. The sum does not make any sense. Maybe a good idea to move private footprint as the first column.

I remember we had different computation for private footprint, one was quick and one was accurate. Does this happen differently on light and heavy dumps? if so we need to show a popup saying how the metric was computed in the UI.

I would still prefer to have all the metrics shown in the Analysis section. It is sometimes critical to look at all the numbers to understand what is going on on linux and android. Since we show all the memory mapped regions, it is better to show "total resident" as the sum of resident size of all the mapped regions.

Side note: If you say some metrics are useless, or not helpful on Mac, we should just not record them and the UI should show only metrics that were recorded.

Comment 4 by ssid@chromium.org, Mar 1 2018

Re comment#2: Yes I don't ever remember using the peak resident metric. But tbh we never tried to work on reducing peak memory usage. Maybe it might be useful at some time in future. I am ok with removing it. There seems to be no easy way to show it hidden.

By removing total resident, the metric will not be seen in the overview section in both detailed and background traces. But, it can still be seen as the total of resident size of mapped regions in analysis section. I am good with that.
Owner: erikc...@chromium.org
Status: Started (was: Untriaged)
Thanks for the feedback, here's the plan:

1) Stop emitting "total resident" from Chrome. Note that on Linux, the information is still available in the analysis view as the sum of "Private Dirty", "Private Clean", "Shared Dirty", "Shared Clean".

And for more users, PMF is a much more useful number.

https://chromium-review.googlesource.com/c/chromium/src/+/940144

2) Stop emitting "PSS", "Private Dirty", "Private Clean", "Shared Dirty", "Shared Clean" on macOS and Windows.

There are no platform-equivalents. The numbers were already not being collected on Windows. Stop attempting to collect them on maCOS.

https://chromium-review.googlesource.com/c/chromium/src/+/943184
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 1 2018

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

commit 3032c633775bcde85fdef8a0136a98a5320fd7e9
Author: erikchen <erikchen@chromium.org>
Date: Thu Mar 01 17:26:05 2018

Stop emitting "resident_set_bytes" in memory-infra traces.

Now that we emit private memory footprint, the former number is not
particularly meaningful.

Bug:  815328 
Change-Id: I85cb76b08ef85073179b5c334c16d4775667b8e6
Reviewed-on: https://chromium-review.googlesource.com/940144
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540197}
[modify] https://crrev.com/3032c633775bcde85fdef8a0136a98a5320fd7e9/services/resource_coordinator/public/cpp/memory_instrumentation/tracing_observer.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Mar 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/406b235a95c475c4bd67cf13f46603a5a2e027e8

commit 406b235a95c475c4bd67cf13f46603a5a2e027e8
Author: erikchen <erikchen@chromium.org>
Date: Fri Mar 02 13:21:17 2018

Fix logic in memory dump event.

Byte stats are Linux specific, and will not be present on macOS and Windows. Fix
the logic in memory dump event to not throw an exception if the field is not
present.

Bug:  chromium:815328 
Change-Id: If12bf94577fc97da2fee45428e0bb5fa099c87f8
Reviewed-on: https://chromium-review.googlesource.com/944004
Reviewed-by: Juan Antonio Navarro Pérez <perezju@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>

[modify] https://crrev.com/406b235a95c475c4bd67cf13f46603a5a2e027e8/telemetry/telemetry/timeline/memory_dump_event.py

Project Member

Comment 8 by bugdroid1@chromium.org, Mar 2 2018

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

commit a1b73669d1e45b07a2d7a4cf0cfd5dbfe08d2a6d
Author: Erik Chen <erikchen@chromium.org>
Date: Fri Mar 02 17:23:35 2018

Stop emitting Linux-specific VMRegion stats on non-Linux derived OSes.

The stats are not meaningful. They were already not being collected on Windows.
Stop attempting to collect macOS stats [which have different semantics].

Bug:  815328 
Change-Id: Ic25eb3c16be47aa87dfb2769fc97a2079b418d34
Reviewed-on: https://chromium-review.googlesource.com/943184
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540544}
[modify] https://crrev.com/a1b73669d1e45b07a2d7a4cf0cfd5dbfe08d2a6d/services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_mac.cc
[modify] https://crrev.com/a1b73669d1e45b07a2d7a4cf0cfd5dbfe08d2a6d/services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_unittest.cc
[modify] https://crrev.com/a1b73669d1e45b07a2d7a4cf0cfd5dbfe08d2a6d/services/resource_coordinator/public/cpp/memory_instrumentation/tracing_observer.cc

Status: Fixed (was: Started)

Sign in to add a comment