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

Issue 783695 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jan 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

7.6% regression in system_health.memory_mobile at 514503:514611

Project Member Reported by alexclarke@chromium.org, Nov 10 2017

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Nov 10 2017

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=783695

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=3c2142177120f5acd5ef8141aa55c6008ff4d0821b237f88cb663e20e10b24d7


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

android-nexus5
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Nov 11 2017


=== BISECT JOB RESULTS ===
Perf regression found but unable to narrow commit range

Build failures prevented the bisect from narrowing the range further.


Bisect Details
  Configuration: android_nexus5_perf_bisect
  Benchmark    : system_health.memory_mobile
  Metric       : memory:chrome:all_processes:reported_by_chrome:v8:effective_size_avg/load_news/load_news_qq
  Change       : 6.99% | 2601906.66667 -> 2783811.33333

Suspected Commit Range
  2 commits in range
  https://chromium.googlesource.com/v8/v8.git/+log/8ba5cfd87374a27894da8db65188980dcc3ff33c..55062ced0be3b21e834f625e77ac4962e7988411


Revision                           Result                  N
chromium@514502                    2601907 +- 67292.1      6        good
chromium@514530                    2610653 +- 31949.6      6        good
chromium@514534                    2611864 +- 26224.5      6        good
chromium@514536                    2603548 +- 53926.1      6        good
chromium@514536,v8@bdcab5f756      2604436 +- 36847.4      6        good
chromium@514536,v8@8ba5cfd873      2611331 +- 26511.4      6        good
chromium@514536,v8@6e1c57eaa9      ---                     ---      build failure
chromium@514536,v8@55062ced0b      2757535 +- 144160       6        bad
chromium@514536,v8@201a40d216      2756815 +- 147100       6        bad
chromium@514537                    2783774 +- 186.569      6        bad
chromium@514544                    2783548 +- 689.812      6        bad
chromium@514557                    2783739 +- 346.533      6        bad
chromium@514611                    2783811 +- 25.5604      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-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=load.news.qq system_health.memory_mobile

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8963319511231557024


For feedback, file a bug with component Speed>Bisection
Project Member

Comment 5 by 42576172...@developer.gserviceaccount.com, Nov 13 2017

Cc: yangguo@chromium.org
Owner: yangguo@chromium.org
Status: Assigned (was: Untriaged)

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

Hi yangguo@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 : Yang Guo
  Commit : 6e1c57eaa906d6d3058fca0c9716a77732b4ef36
  Date   : Tue Nov 07 11:45:30 2017
  Subject: Remove UnseededNumberDictionary.

Bisect Details
  Configuration: android_nexus5_perf_bisect
  Benchmark    : system_health.memory_mobile
  Metric       : memory:chrome:all_processes:reported_by_chrome:v8:effective_size_avg/load_news/load_news_qq
  Change       : 7.13% | 2598786.0 -> 2784089.33333

Revision                           Result                  N
chromium@514400                    2598786 +- 53476.5      6      good
chromium@514525                    2603308 +- 63559.6      6      good
chromium@514533                    2613473 +- 30667.7      6      good
chromium@514535                    2610211 +- 35316.0      6      good
chromium@514536                    2610050 +- 21796.6      6      good
chromium@514536,v8@bdcab5f756      2606633 +- 49916.3      6      good
chromium@514536,v8@8ba5cfd873      2612573 +- 22270.7      6      good
chromium@514536,v8@6e1c57eaa9      2764220 +- 165493       9      bad       <--
chromium@514536,v8@201a40d216      2783763 +- 364.447      6      bad
chromium@514537                    2757510 +- 144132       6      bad
chromium@514541                    2783804 +- 205.056      6      bad
chromium@514557                    2783722 +- 530.381      6      bad
chromium@514588                    2783722 +- 230.946      6      bad
chromium@514650                    2784089 +- 160.864      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-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=load.news.qq system_health.memory_mobile

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8963051738825710432


For feedback, file a bug with component Speed>Bisection
Cc: perezju@chromium.org u...@chromium.org
yangguo: did you have a chance to look at this yet?

Also cc-ing test owner perezju and v8 memory owner ulan: Looking at the graph, this metric seems to flip back and forth a lot, are regression bugs like this valuable? Should we increase the threshold for this metric?
> Looking at the graph, this metric seems to flip back and forth a lot, are regression bugs like this valuable? Should we increase the threshold for this metric?

Although the magnitude of this regression looks small (~100KiB), looking at the longer trend across time:
https://chromeperf.appspot.com/report?sid=b588995409420a670b0237ff18b5478e27480059aec74fb323552d1c2487cc51&start_rev=449888&end_rev=530445

it looks like we've reduced about 1MiB usage thanks to many such small drops in about 6 months.

So I don't think we should ignore the small regressions either, as they could add up in the wrong direction. If we want to set a threshold anyway, I would suggest alerting on anything at least 64KiB and above (for v8 metrics).
The CL removed UnseededNumberDictionary, which uses 2 slots per entry, in favor of consistently using SeededNumberDictionary, which uses 3 slots per entry. That would explain the memory increase.

I'll work on a fix.
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/3857b44e69bb87c2cf481255e57fe2080c761e75

commit 3857b44e69bb87c2cf481255e57fe2080c761e75
Author: Yang Guo <yangguo@chromium.org>
Date: Thu Jan 25 13:49:23 2018

Introduce SimpleNumberDictionary.

This is somewhat of a revival of what used to be
UnseededNumberDictionary. The difference to NumberDictionary is that
each entry only has two fields (no field for property details) and there
is no header field for a bitfield.

The reason for this change is memory regression introduced when we
removed UnseededNumberDictionary (6e1c57eaa9). We now use
SimpleNumberDictionary for
- slow template instantiation cache
- code stubs table
- value serializer map
- stack frame cache
- type profile source positions

R=ishell@chromium.org, ulan@chromium.org

Bug:  chromium:783695 
Change-Id: I3cd32e485060bb379fb2279eeefbbbded7455f0e
Reviewed-on: https://chromium-review.googlesource.com/885811
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50869}
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/api-natives.cc
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/bootstrapper.cc
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/code-stubs.cc
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/contexts.h
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/factory.cc
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/factory.h
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/feedback-vector.cc
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/globals.h
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/heap/heap.cc
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/heap/heap.h
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/heap/setup-heap-internal.cc
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/isolate.cc
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/objects-inl.h
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/objects.cc
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/objects.h
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/objects/code.h
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/objects/dictionary.h
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/value-serializer.cc
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/src/value-serializer.h
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/tools/gen-postmortem-metadata.py
[modify] https://crrev.com/3857b44e69bb87c2cf481255e57fe2080c761e75/tools/v8heapconst.py

Status: Fixed (was: Assigned)
Fix has been landed, but does not show up on the graphs.

The reason for this is probably that memory changes caused by V8's heap happen in jumps, as V8's heap grows page by page. The initial regression somehow pushed the memory use over a page, but since a lot of things changed in between, the fix did not immediately have any effect. However, we are already looking better memory-wise at this point.
Status: Verified (was: Fixed)

Sign in to add a comment