6.5% regression in speedometer2 at 519946:520079 |
|||||||||||
Issue descriptionSee the link to graphs below.
,
Nov 30 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8961470627177962128
,
Dec 1 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 : 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
,
Dec 1 2017
mlippautz@, I'm suspecting your change for the perf regression, can you take a look?
,
Dec 1 2017
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.
,
Dec 5 2017
,
Dec 5 2017
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)?
,
Dec 6 2017
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.
,
Dec 6 2017
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.
,
Dec 6 2017
Quick update: I seem to be getting back the performance by not emitting the write barrier on initializing stores. Will prepare a CL now.
,
Dec 6 2017
,
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
,
Dec 11 2017
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
,
Dec 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fad8f267ba2718c5e6a4011263835be8cc428a16 commit fad8f267ba2718c5e6a4011263835be8cc428a16 Author: Michael Lippautz <mlippautz@chromium.org> Date: Wed Dec 13 17:57:53 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-Commit-Position: refs/heads/master@{#523812} [modify] https://crrev.com/fad8f267ba2718c5e6a4011263835be8cc428a16/third_party/WebKit/Source/platform/heap/IncrementalMarkingTest.cpp [modify] https://crrev.com/fad8f267ba2718c5e6a4011263835be8cc428a16/third_party/WebKit/Source/platform/heap/Member.h [modify] https://crrev.com/fad8f267ba2718c5e6a4011263835be8cc428a16/third_party/WebKit/Source/platform/heap/ThreadState.cpp
,
Dec 13 2017
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.
,
Dec 13 2017
,
Dec 13 2017
,
Dec 14 2017
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
,
Dec 18 2017
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 |
|||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Nov 30 2017