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

Issue 790691 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Regression

Blocking:
issue 757440



Sign in to add a comment

6.5% regression in speedometer2 at 519946:520079

Project Member Reported by lfg@chromium.org, Nov 30 2017

Issue description

See the link to graphs below.
 
Project Member

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

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

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


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

android-nexus5

=== 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    : speedometer2
  Metric       : Vanilla-ES2015-TodoMVC/Speedometer2
  Change       : 6.77% | 2632.47888889 -> 2810.68333333

Suspected Commit Range
  2 commits in range
  https://chromium.googlesource.com/chromium/src/+log/4f59a790e19f97e8bbdbb6e7960883c1e1cf0234..c47b4d110a697afa6a6ee4d4748028312c65ec19


Revision             Result                  N
chromium@519945      2632.48 +- 27.2807      6        good
chromium@520013      2620.76 +- 46.2711      6        good
chromium@520046      2631.66 +- 74.9054      6        good
chromium@520055      2618.61 +- 54.2034      6        good
chromium@520060      2625.47 +- 50.595       6        good
chromium@520061      ---                     ---      build failure
chromium@520062      2801.34 +- 102.354      6        bad
chromium@520063      2801.41 +- 39.3294      6        bad
chromium@520079      2810.68 +- 63.8685      6        bad

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=Speedometer2 speedometer2

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

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


For feedback, file a bug with component Speed>Bisection

Comment 4 by lfg@chromium.org, Dec 1 2017

Owner: mlippautz@chromium.org
mlippautz@, I'm suspecting your change for the perf regression, can you take a look?
Components: -Blink>JavaScript Blink>MemoryAllocator>GarbageCollection
Status: Assigned (was: Untriaged)
Thanks for triaging.

The CL enables the write barrier and we see the throughput trade off for potentially marking incrementally in Oilpan.

Lets see if more regressions bubble in. 
Blocking: 757440
Cc: haraken@chromium.org keishi@chromium.org
It looked like the original change was made to make the jumbo build easier. Is it possible to avoid paying the cost of the write barrier until we are ready to ship incremental marking (and we are making a tradeoff for a performance win on GC pause time or something else)?
Now that optimizing Speedometer is P0, it's not acceptable to regress it by 6%.

Since Michael is now working on reducing the overhead of write barriers, I'd propose reverting the CL for now and reland it after Michael's optimizations land.

Cc: hpayer@chromium.org
Labels: -Pri-2 Pri-1
Status: Started (was: Assigned)
Lets not make this a general discussion about GC trade offs here. The overall regression is 2.5% (as discussed in yesterday's meeting). 

Since I am actively working on this issue and want to have perf coverage I'd rather fix on ToT. We can always fix this before a branch cut by simply guarding it.

Readjusting priorities to reflect this.
Quick update: I seem to be getting back the performance by not emitting the write barrier on initializing stores. 

Will prepare a CL now.
results-wb-optimization.html
1.1 MB View Download
Cc: pmeenan@chromium.org mlippautz@chromium.org
 Issue 792061  has been merged into this issue.
Project Member

Comment 12 by bugdroid1@chromium.org, Dec 7 2017

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

commit faf0c6646d65d165018b9af7331033b4c358e7ba
Author: Michael Lippautz <mlippautz@chromium.org>
Date: Thu Dec 07 01:56:56 2017

[oilpan] Avoid write barrier on initilizing stores

Initializing stores (through constructors) can assume that objects are
not yet marked and thus don't require write barriers.

This CL breaks placement new for Member which can violate the assumption
that the enclosing object is white. A follow up will fix this.

Bug:  chromium:757440 ,  chromium:790691 
Change-Id: Id0ef91ba1887837593603eaf73f56a127f8fda0a
Reviewed-on: https://chromium-review.googlesource.com/812344
Commit-Queue: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522303}
[modify] https://crrev.com/faf0c6646d65d165018b9af7331033b4c358e7ba/third_party/WebKit/Source/platform/heap/Member.h

Cc: -mlippautz@chromium.org
A few regressions recovered after landing the optimization. There's some left but also a second regression that was eating up the improvements of my optimization, at least on micro benchmarks. See  issue 792444 .

While individual sub items may still show regressions, the total score should have mostly recovered now. Regular nexus 5 is running at half the CPU speed so it should not serve as a guard here.

For improving further, I also tried using TLS again for the initial guard (results: [1]; CL: [2]). This just confirms (again) that TLS lookup is slower than loading page-aligned flags. Ideally, we would store that flag somewhere in a global that is always in the cache. For V8's GC we refer to page flags because of a generational barrier. On Android, the GC gets a whole register for this check.

[1]: https://00e9e64bacbe233de03321bc05f7284219cec9d10e1438c9f6-apidata.googleusercontent.com/download/storage/v1/b/chromium-telemetry/o/html-results%2Fresults-2017-12-10_14-05-26?qk=AD5uMEsFj_9qYwhYdkesfMTFQGnXcCadE1bhduhsODL9VWEkLOzwzqWd6rYNncfWjvdD-LHt2d6u6_Ij6LMPe2UPa2KuGv8wQQP7AXasdBEeFVWipgURL-9OFwodoJkw9m77yp36ckdn6VKgDubl6C5qgqeD1jKTh237vfWv_oJ7THQWCda9jzmaSakRqQhbpWcSA5cB6SindtHEnJQvXKIYff6JusPiQHDjKiRqftJ-_D_ArCjFupHhNGthVfO24EajbYe7Hl76nburbfTp3LeiAW5q46Q-cZ84ijVCQFgv5SUbewdUDXtk8VvSMP0KLKVe0VxYKEuwGQr5sGzuuH4thipA0QU6jZnnvxhmoP-ym9m70VFMZhkRZVrjIWCnuNlZNUC48hAa2HyrCVva1H_5imKbhoHtNDRzWp41vfAxps-YNb5X8w_YwkYiGe7x9xTNk1AD800jDdKmxGd0cCIMtKXUKtNzMMoCx7y648p9iIx4ay6Hr_neqsGYN75jiBwL5zmxsHzRnwWpxi6VBH9ynZTyhs_A7pQJt8UEeyY3aDmJJQXqbA-fZnHAFTiGcWvqLxmD46PuRA_wit8UljY8lB67P_HRy_pdq7BHwmKpUUSyx2GyS_UG5hx0poGej9kk7YM2HxXR-I9xuWMI4O8wZlpyrUyN08-4T0cedoPZ_C-unfzRGXgI8aaSJ0D8WMIAZVdx5BVSDk0meIKu7WqnST7jsC-pYegWC1Ti44Gtm8BMO4q8hD1vqK_rN-LkIttTSF9_3_BlGngY0B_jF5uPus5wssMYmw#r=TOT&s=%25%CE%94avg&g=name&c=0
[2]: https://chromium-review.googlesource.com/c/chromium/src/+/817742
Cc: hablich@chromium.org
Labels: Merge-Request-64
Requesting backmerge of #14 to M64. The backmerge is definitely safe as the feature is not enabled by default. This will recover any potential performance regressions.
Labels: OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
Status: Fixed (was: Started)
Project Member

Comment 18 by sheriffbot@chromium.org, Dec 14 2017

Labels: -Merge-Request-64 Hotlist-Merge-Approved Merge-Approved-64
Your change meets the bar and is auto-approved for M64. Please go ahead and merge the CL to branch 3282 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 19 by bugdroid1@chromium.org, Dec 18 2017

Labels: -merge-approved-64 merge-merged-3282
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/acb1249f4b9251e882f303903bdc50c6f03f0bac

commit acb1249f4b9251e882f303903bdc50c6f03f0bac
Author: Michael Lippautz <mlippautz@chromium.org>
Date: Mon Dec 18 13:39:32 2017

[oilpan] Avoid building with incremental marking

Tbr: haraken@chromium.org
Bug:  chromium:757440 ,  chromium:790691 
Change-Id: I1a660cc40c1f79713dd98948c0efd4477eff958b
Reviewed-on: https://chromium-review.googlesource.com/823844
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#523812}(cherry picked from commit fad8f267ba2718c5e6a4011263835be8cc428a16)
Reviewed-on: https://chromium-review.googlesource.com/829073
Cr-Commit-Position: refs/branch-heads/3282@{#261}
Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840}
[modify] https://crrev.com/acb1249f4b9251e882f303903bdc50c6f03f0bac/third_party/WebKit/Source/platform/heap/IncrementalMarkingTest.cpp
[modify] https://crrev.com/acb1249f4b9251e882f303903bdc50c6f03f0bac/third_party/WebKit/Source/platform/heap/Member.h
[modify] https://crrev.com/acb1249f4b9251e882f303903bdc50c6f03f0bac/third_party/WebKit/Source/platform/heap/ThreadState.cpp

Sign in to add a comment