New issue
Advanced search Search tips

Issue 697980 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug-Regression



Sign in to add a comment

2.5% regression in system_health.memory_mobile at 454055:454099

Project Member Reported by sullivan@chromium.org, Mar 2 2017

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=697980

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgpLmchwgM


Bot(s) for this bug's original alert(s):

android-webview-nexus5X
Cc: mlamouri@chromium.org
Owner: mlamouri@chromium.org

=== Auto-CCing suspected CL author mlamouri@chromium.org ===

Hi mlamouri@chromium.org, the bisect results pointed to your CL, please take a look at the
results.


=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : mlamouri
  Commit : 8daac32c17f494d496fb2ca3768d67d9bd77db5e
  Date   : Wed Mar 01 22:10:03 2017
  Subject: Media: fix memory leak because of the document holding on an EventListener.

Bisect Details
  Configuration: android_webview_arm64_aosp_perf_bisect
  Benchmark    : system_health.memory_mobile
  Metric       : memory:webview:all_processes:reported_by_os:system_memory:native_heap:proportional_resident_size_avg/load_media/load_media_dailymotion
  Change       : 2.68% | 57237405.3333 -> 58770504.0

Revision             Result                   N
chromium@454054      57237405 +- 1067354      6      good
chromium@454057      57762717 +- 998447       6      good
chromium@454059      57448349 +- 1080994      6      good
chromium@454060      58755144 +- 694903       6      bad       <--
chromium@454066      58880072 +- 604226       6      bad
chromium@454077      58838088 +- 980235       6      bad
chromium@454099      58770504 +- 718719       6      bad

Please refer to the following doc on diagnosing memory regressions:
  https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=load.media.dailymotion system_health.memory_mobile

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8986206717238888784

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5085590590062592


| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Speed>Bisection.  Thank you!
Cc: haraken@chromium.org
Labels: OS-Android
+haraken, this is actually an interesting regression. mlamouri@ and I have been looking into this for a while.
So there is definitely a regression in the blink_gc cagegory, which shows on different devices. So it's not noise.
The regression is definitely in the oilpan total effective size, see for instance these charts:
https://chromeperf.appspot.com/report?sid=3bb3bf51a3221505b09412715460db104d40f4b14d9c79af230ee6e3d53b5d07&start_rev=451269&end_rev=454289

The interesting part is that, by looking at the traces, there is no regression in the "allocated_objects_size". The entire difference before vs after the CL is in the "free_size" column.
Essentially it looks like, after this CL, oilpan retains 700KB more stuff in its freelists without releasing that memory to the system.
Can somebody in the oilpan team take a look to the traces and see if they can understand more?

trace before: https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/trace-file-id_17-2017-03-01_15-03-52-49817.html

trace after: https://console.developers.google.com/m/cloudstorage/b/chrome-telemetry-output/o/trace-file-id_17-2017-03-01_20-18-52-55589.html
Components: Blink>MemoryAllocator
Status: Started (was: Untriaged)
My theory is that using the DOMInserted* and DOMRemoved* events has a significant overhead. I have a CL up that uses the C++ callbacks instead. Locally, it seems to make things slightly better. Let see if landing this will reverse the free_size increase.
Project Member

Comment 7 by bugdroid1@chromium.org, Mar 4 2017

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

commit 8e679697d635baa92d4927b2b4e838f125266af1
Author: mlamouri <mlamouri@chromium.org>
Date: Sat Mar 04 13:57:19 2017

Media Controls: do not use events but C++ callbacks for insertion/removal from document.

For some reasons, the overhead of the events create some oilpan
allocated free memory. It's not a memory leak but it increases the
visible memory usage of Chrome for some perf bots. Using C++ callbacks
resolve the issue and should be fairly similar as far as the
functionality goes.

BUG= 697980 

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

[modify] https://crrev.com/8e679697d635baa92d4927b2b4e838f125266af1/third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
[modify] https://crrev.com/8e679697d635baa92d4927b2b4e838f125266af1/third_party/WebKit/Source/core/html/shadow/MediaControls.h
[modify] https://crrev.com/8e679697d635baa92d4927b2b4e838f125266af1/third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp
[modify] https://crrev.com/8e679697d635baa92d4927b2b4e838f125266af1/third_party/WebKit/Source/core/html/shadow/MediaControlsOrientationLockDelegate.cpp

Status: WontFix (was: Started)
Sorry, this bug slipped off my mind.

I chatted with mlamouri offline. My best guess is that mlamouri's CL *by accident* changed an object allocation workload and thus changed the way Oilpan allocates things on the free list. The fact that the CL is not regression allocated_objects_size indicates that this is not a real regression. If that is the case, it's not really worth digging into the issue.

Marking it as wontfix.


Labels: Performance-Memory

Sign in to add a comment