Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 691033 3.8%-7.5% regression in system_health.memory_mobile at 449423:449504
Starred by 1 user Reported by majidvp@google.com, Feb 10 Back to list
Status: Assigned
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment
See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=691033

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgmKbKuwsM,agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgIDgmJLn-AgM


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

android-webview-nexus6
Cc: japhet@chromium.org
Owner: japhet@chromium.org

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

Hi japhet@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 : japhet
  Commit : ffefd4173f178d76449fc51f5dc22bfd7e120e03
  Date   : Thu Feb 09 22:25:56 2017
  Subject: Actually handle the Vary header in blink, rather than punting on sight.

Bisect Details
  Configuration: android_webview_nexus6_aosp_perf_bisect
  Benchmark    : system_health.memory_mobile
  Metric       : memory:webview:all_processes:reported_by_chrome:v8:effective_size_avg/load_media/load_media_9gag
  Change       : 6.54% | 14134364.8889 -> 15091096.0

Revision             Result                   N
chromium@449422      14134365 +- 272606       9       good
chromium@449430      14102072 +- 354500       9       good
chromium@449434      14035520 +- 377421       9       good
chromium@449436      14226880 +- 1122881      14      good
chromium@449437      14057307 +- 548933       9       good
chromium@449438      14917434 +- 1250773      9       bad       <--
chromium@449504      15091096 +- 211403       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.9gag system_health.memory_mobile

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

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


| 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!
Status: WontFix
I verified that https://crrev.com/ffefd4173f178d76449fc51f5dc22bfd7e120e03 is responsible for the change, and that it's specifically related to the change in caching behavior, not to the header parsing portion of my change. I'm fairly certain this is a measurement/timing change, not an actual memory regression.

Reading through the test harness, (third_party/catapult/telemetry/telemetry/internal/actions/navigate.py and MeasureMemory() in third_party/catapult/telemetry/telemetry/internal/actions/action_runner.py) I believe the flow of the test is;
* Wait for document.readyState to be at least interactive
* Wait for rAF
* GC
* Measure memory

My patch causes us to use more resources from blink's in-memory cache, rather than going to the browser process cache. It's entirely possible, e.g., if images are in the cache, that we will load them immediately from the cache, and this will in turn lead to additional memory allocations. This would happen immediately rather than requiring an IPC roundtrip to the browser process, and could certainly happen before document.readyState == interactive. Images are irrelevant to document.readyState, so if more images load before readyState == interactive, this would lead to more memory in use at the time of measurement.

Given this, I'm inclined to declare this WontFix. Feel free to reopen if there are objections.

Cc: perezju@chromium.org
The explanation is plausible. Have you looked to see if there is improvement elsewhere in other metrics? I would have imagined this change to perhaps improve metrics that measure first meaningfull paint. (Though I am not sure if we have any such metric)

perezju@: Are you happy with this resolution? See comment #4. 
Cc: benhenry@chromium.org primiano@chromium.org
Status: Assigned
+benhenry, +primiano

I think we should make sure we understand better what is going on. The largest increases caught by these alerts are in the order of ~20 MB, so that will be hard to explain if these affect system health measurements.

A couple of large clean regressions found here:

[1]: https://chromeperf.appspot.com/report?sid=4c4f9ad55a2fe122857f117af42151bffed4b0c46cf785ae1a980785d6548321&rev=449546

[2]: https://chromeperf.appspot.com/report?sid=c49f2707110896c1e4a99767cd0e6bf24e7ea4506dce663d80f621e547650c70&rev=449638
I think that the WontFix was a bit premature.
First of all, memory is not the only thing regressing here. As clearly visible by the link in #1 there are regressions in v8 gc time as well as regressions in timeToFirstContentfulPaint_avg .

As regards memory, the explanation is plausible, but I would like to see a trace on the pages that are regressing that proves it.
You should be able inject another memory measurement after some seconds by tweaking the code you found.


Labels: Performance-Memory
Sign in to add a comment