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

Issue 742958 link

Starred by 2 users
Status: Fixed
Owner:
█ hjd@chromium.org
Last visit > 30 days ago
Closed: Jul 2017
Cc:
ssid@chromium.org
█ benhenry@chromium.org
erikc...@chromium.org
primiano@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Double counting rss_anon_bytes when computing private footprint

Project Member Reported by hjd@chromium.org, Jul 14 2017 
A refactor (crrev.com/c/555497) I made to ProcessMetricsMemoryDumpProvider
which split logic out by platform introduced a bug. The bug caused us to
double count rss anon bytes and never count vm swap bytes when computing
the 'private footprint' UMA on all processes on Linux and Android. This
produced a large increase in those UMA metrics.

UMA: https://uma.googleplex.com/timeline_v2?sid=0ab9c44731051a88ec6911ce5879cacd
Commit: https://chromium.googlesource.com/chromium/src/+/36eedd751a6104eea2d3d25c5a8ff25b9bd13510

Before:
-  footprint.rss_anon_bytes = (resident_pages - shared_pages) * page_size;
-  footprint.vm_swap_bytes = process_metrics_->GetVmSwapBytes();
-  pmd->process_totals()->SetPlatformPrivateFootprint(footprint);

After:
+  const static size_t page_size = base::GetPageSize();
+  uint64_t rss_anon_bytes = (resident_pages - shared_pages) * page_size;
+  uint64_t vm_swap_bytes = (resident_pages - shared_pages) * page_size;

Comment 1 by primiano@chromium.org, Jul 14 2017 

Copy-pasting myself from the CL:

at this point we should have a test. Testing memory measurement code is always hard, but we can try to do something like:

allocate a big buffer (say 10 MB)
check that footprint goes up by > 7 MB (leave ~3 MB to compensate for other things happening on the test / system / swap)
free the big buffer
check footprint that goes down to original value +- 3 MB
Given the important of this though I would do the test separately
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 14 2017 

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

commit bd87bf3514c2d887d21c0e21ef466b560c976ed7
Author: Hector Dearman <hjd@google.com>
Date: Fri Jul 14 12:07:01 2017

memory-infra: Stop double counting rss_anon_bytes when computing private footprint

A refactor (crrev.com/c/555497) I made to ProcessMetricsMemoryDumpProvider
which split logic out by platform introduced a bug. The bug caused us to
double count rss anon bytes and never count vm swap bytes when computing
the 'private footprint' UMA on all processes on Linux and Android. This
produced a large increase in those UMA metrics.

This fixes the bug.

Bug:  742958 
Change-Id: I893774e95154f50be0f30698b17c01ec4bfbcb1d
Reviewed-on: https://chromium-review.googlesource.com/571804
Commit-Queue: Hector Dearman <hjd@chromium.org>
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486742}
[modify] https://crrev.com/bd87bf3514c2d887d21c0e21ef466b560c976ed7/services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_linux.cc
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 19 2017 

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

commit efe60ec81ccfea801c059f87d5382200f30e7405
Author: Hector Dearman <hjd@chromium.org>
Date: Wed Jul 19 14:51:52 2017

Revert "memory-infra: Add an end-to-end test of private footprint"

This reverts commit 8779e5365c7b389822075b7eb7b35697b36d9954.

Reason for revert: Breaks a test: https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20MSan%20Tests/builds/2673

Original change's description:
> memory-infra: Add an end-to-end test of private footprint
> 
> Bug:  742958 
> Change-Id: I2908ad0794835b1ad002da3a4b9be7c3578cdce5
> Reviewed-on: https://chromium-review.googlesource.com/574029
> Commit-Queue: Hector Dearman <hjd@chromium.org>
> Reviewed-by: Primiano Tucci <primiano@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#487834}

TBR=primiano@chromium.org,erikchen@chromium.org,hjd@chromium.org

Change-Id: I3d309673b0cf4ce489d6ee535975ea1bc049e782
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  742958 
Reviewed-on: https://chromium-review.googlesource.com/576184
Reviewed-by: Hector Dearman <hjd@chromium.org>
Commit-Queue: Hector Dearman <hjd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487860}
[delete] https://crrev.com/ab08362cb469d48e79bee165b56d7bf962382490/content/browser/tracing/memory_instrumentation_browsertest.cc
[modify] https://crrev.com/efe60ec81ccfea801c059f87d5382200f30e7405/content/test/BUILD.gn
[modify] https://crrev.com/efe60ec81ccfea801c059f87d5382200f30e7405/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.cc
[modify] https://crrev.com/efe60ec81ccfea801c059f87d5382200f30e7405/services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h

Comment 6 by hjd@chromium.org, Jul 20 2017 

This is fixed now and an end-to-end test has been added which would have caught this bug.

Comment 7 by hjd@chromium.org, Jul 21 2017

Status: Fixed (was: Started)
►

Sign in to add a comment