Issue metadata
Sign in to add a comment
|
84.7%-559.4% regression in v8.browsing_desktop at 513534:513616 |
||||||||||||||||||||
Issue descriptionSTrange. Must bisect this one.
,
Nov 7 2017
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/1692b046f80000
,
Nov 21 2017
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/16c565c3f80000
,
Nov 21 2017
📍 Found a significant difference after 1 commit. https://pinpoint-dot-chromeperf.appspot.com/job/16c565c3f80000 Roll HarfBuzz to 1.6.3 By drott@chromium.org · Thu Nov 02 17:49:34 2017 chromium @ d9338280305c81f3dac8aa1a3da3e459e3f341f9 Understanding performance regressions: http://g.co/ChromePerformanceRegressions
,
Nov 21 2017
Probably a duplicate of issue 781794 - mvstanton@, what are these renderer_eqt benchmarks? What do they compute?
,
Nov 22 2017
renderer_eqt are the metrics to measure the overall jank. They measure the longest interval in a 500ms window that cannot be interrupted. (https://cs.chromium.org/chromium/src/third_party/catapult/tracing/tracing/metrics/system_health/expected_queueing_time_metric.html). We measure the overall (not just the startup) jank using total:500ms_window:renderer_eqt_max metric and also compute various related metrics for the event specific contribution of the jank. For example, total:500ms_window:renderer_eqt:v8_max computes the V8 contribution for the jank. There are also metrics like interactive:500ms_window:renderer_eqt that measure jank during the startup time. So with the changes in the startup time I can see the interactive:500ms_window:renderer_eqt getting impacted, but total:renderer_eqt should not.
,
Nov 22 2017
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/12cf3dd3f80000
,
Nov 22 2017
I am starting another bisect, looks like v8 execute (the actual javascript execution time) bucket is the one that regressed. Though this looks a bit strange to me that only one page has regressed quite significantly.
,
Nov 22 2017
Re comment8: That was only for tech/discourse_infinite_scroll page. The others (twitter and hackernews) don't seem to be related to V8. The v8 contribution of jank metrics haven't changed only the overall jank regressed.
,
Nov 22 2017
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/1680aaabf80000
,
Nov 22 2017
It does look strange, please see the comments in issue 781794 and blocking issue 786441 , where I have severe difficulties measuring this locally or on bots. I am running $ tools/perf/run_benchmark try linux v8.browsing_desktop --story-filter='browse:news:hackernews' --pageset-repeat=10 On this benchmark and storyset the pinpoint job found a difference in the 'max' statistics for total:500ms_window:renderer_eqt. How reliable are the max values? bisect_perf_runs: https://chromium-review.googlesource.com/#/c/chromium/src/+/785670 for Rollback to 1.5.1 plus ICU fixes. https://chromium-review.googlesource.com/c/chromium/src/+/785652 for master, just touching the README.chromium.
,
Nov 23 2017
📍 Found a significant difference after 1 commit. https://pinpoint-dot-chromeperf.appspot.com/job/1680aaabf80000 Roll HarfBuzz to 1.6.3 By drott@chromium.org · Thu Nov 02 17:49:34 2017 chromium @ d9338280305c81f3dac8aa1a3da3e459e3f341f9 Understanding performance regressions: http://g.co/ChromePerformanceRegressions
,
Nov 28 2017
📍 Found a significant difference after 1 commit. https://pinpoint-dot-chromeperf.appspot.com/job/12cf3dd3f80000 Roll HarfBuzz to 1.6.3 By drott@chromium.org · Thu Nov 02 17:49:34 2017 chromium @ d9338280305c81f3dac8aa1a3da3e459e3f341f9 Understanding performance regressions: http://g.co/ChromePerformanceRegressions
,
Nov 30 2017
I am running another 3 perf_bisects: 1.6.3 as is: https://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bisect/builds/7824 1.6.3, but reverting the hb_set changes: https://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bisect/builds/7825 Rollback to 1.5.1: https://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bisect/builds/7826
,
Nov 30 2017
Running my benchmark from https://chromium-review.googlesource.com/c/chromium/src/+/788393, modified to do not 100words runs but 1 word runs, doesn't reveal any striking differences: 1.6.3, word by word 30287 ms 34072 ms 29245 ms 28984 ms 32252 ms 1.6.3, set-revert, word-by-word 31008 ms 29697 ms 28962 ms 32845 ms 31732 ms
,
Dec 1 2017
Behdad, looking at the perf_bisects works as follows: You click on of the links in comment 14, then find HTML results, then tick storysetRepeats, then in the search field enter "eqt", and look at the 'total:500ms_window:renderer_eqt' benchmark, then unfold the repeats by clicking on ▶. It's important to look at the individual runs, as there seems to be occasional noise spraying into some of the runs, slowing them down unexpectedly. Now for first benchmark run, which is only touching the readme, otherwise same as 1.6.3/trunk this tooling issue manifests itself and the very first of the runs is too slow, but the next ones are about even with trunk/no changes. For 1.6.3 plus set revert most of the runs improve around 86-88%, so very similar to the full revert. For 1.5.1 rollback, the metric improves roughly 83%. mythria@, any ideas where the occasional outliers come from in this metric? Behdad, I can also confirm that the FreeType change about hb_set from http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/ChangeLog?id=90461c0137e5369fc0fdcaffc4abd640a9591cd3 is already rolled in. My current approach would be to cherry-pick/apply the revert of the set_changes and observe whether the benchmark metrics here normalize again, wdyt?
,
Dec 1 2017
Dominik and I drilled more on this offline and found that slowdown is probably coming from Blink's wordcache code calling hb_ot_layout_collect_glyphs() and that is slowed down by new set implementation. I just pushed this fix to HarfBuzz: https://github.com/harfbuzz/harfbuzz/commit/438c325a256f040c6be840924ed42dcbcd8a049a
,
Dec 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8db92d3744fe418e7871e863ad53b58b0209e412 commit 8db92d3744fe418e7871e863ad53b58b0209e412 Author: Dominik Röttsches <drott@chromium.org> Date: Fri Dec 01 21:41:48 2017 Patch hb_set implementation back to pre 1.6.1 implementation Perf bisects indicate that the hb_set changes may be the reason for seing performance regressions in issues 781794 and 782220 . This is somewhat speculative as there were issues with reproducibility in the perf_bisect runs. Results from the main perf bot graphs should help figure out whether the hb_set changes were the culprit here. Bug: 782220 , 781794 Change-Id: I5462b37503ca76a299c99cf41388d0d195722358 Tbr: behdad@chromium.org Reviewed-on: https://chromium-review.googlesource.com/804374 Reviewed-by: Dominik Röttsches <drott@chromium.org> Commit-Queue: Dominik Röttsches <drott@chromium.org> Cr-Commit-Position: refs/heads/master@{#521069} [modify] https://crrev.com/8db92d3744fe418e7871e863ad53b58b0209e412/third_party/harfbuzz-ng/README.chromium [modify] https://crrev.com/8db92d3744fe418e7871e863ad53b58b0209e412/third_party/harfbuzz-ng/src/hb-set-private.hh [modify] https://crrev.com/8db92d3744fe418e7871e863ad53b58b0209e412/third_party/harfbuzz-ng/src/hb-set.cc
,
Dec 1 2017
And some more: https://github.com/harfbuzz/harfbuzz/commit/9d0194b3a8e0c562249337fa0cf4d72e89334263 These two should do it.
,
Dec 2 2017
Issue 781794 has been merged into this issue.
,
Dec 4 2017
The graphs from this issue and issue 781794 have recovered, closing this. The takeaway is that most likely https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp?sq=package:chromium&l=257 HasSpaceInTable and HasSpaceInLigaturesOrKerning are hammering the hb_set implementation in a different way than how it's used for shaping inside HarfBuzz. That's in line with the observation that we do not see shaping slowdowns, but we only see a slow down from the Blink side. Behdad added optimizations for the hb_set_add_range function, and we should be able to reduce the number of calls to the above two functions. With those fixes we should be able to reland and roll HarfBuzz.
,
Dec 4 2017
,
Dec 4 2017
,
Dec 5 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 7 2017
After the HarfBuzz roll to 1.7.2 ( https://chromium-review.googlesource.com/c/chromium/src/+/807986 ) the graphs from this issue and issue 781794 are going back up. I will now try to land https://chromium-review.googlesource.com/c/chromium/src/+/809106 (Move HasSpaceInLigaturesOrKerning to HarfBuzzFace) which we believe can improve the situation.
,
Dec 8 2017
Unfortunately, the attempt from #25 did not have the effect we had hoped for. I am patching the set implementation back to pre 1.6.1.
,
Dec 8 2017
,
Dec 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b071f305c1699ff51c09af22d97953ce1c5adeb1 commit b071f305c1699ff51c09af22d97953ce1c5adeb1 Author: Dominik Röttsches <drott@chromium.org> Date: Fri Dec 08 11:02:20 2017 Patch hb_set implementation back to pre 1.6.1 implementation again Perf bisects indicate that the hb_set changes may be the reason for seing performance regressions in issues 781794 and 782220 . The previous CL for patching the set implementation back [1] shows that this makes the performance graphs recover. In [2] we reduced the number of calls to HasSpaceInLigaturesOrKerning in an attempt to see whether this can address the performance regressions. However, the graphs in [3] show that even after Chromium revision 522394 (which is the revision for [3]) the graphs are not recovering. [1] https://chromium-review.googlesource.com/c/chromium/src/+/804374 [2] https://chromium-review.googlesource.com/c/chromium/src/+/809106 [3] https://chromeperf.appspot.com/group_report?bug_id=781794 Bug: 782220 Change-Id: Ib4708dbba70a36d1f7659ab00dfe8222e8623d8b Tbr: eae, kojii, behdad Reviewed-on: https://chromium-review.googlesource.com/816795 Reviewed-by: Dominik Röttsches <drott@chromium.org> Reviewed-by: Koji Ishii <kojii@chromium.org> Commit-Queue: Dominik Röttsches <drott@chromium.org> Cr-Commit-Position: refs/heads/master@{#522757} [modify] https://crrev.com/b071f305c1699ff51c09af22d97953ce1c5adeb1/third_party/harfbuzz-ng/README.chromium [modify] https://crrev.com/b071f305c1699ff51c09af22d97953ce1c5adeb1/third_party/harfbuzz-ng/src/hb-set-private.hh [modify] https://crrev.com/b071f305c1699ff51c09af22d97953ce1c5adeb1/third_party/harfbuzz-ng/src/hb-set.cc
,
Dec 8 2017
,
Dec 11 2017
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e223d5aee70a70447a67910b7c8315c6aad777af commit e223d5aee70a70447a67910b7c8315c6aad777af Author: Dominik Röttsches <drott@chromium.org> Date: Tue Dec 12 13:37:26 2017 Revert patching hb_set back to old impl, but increase page size This reverts commit b071f305c1699ff51c09af22d97953ce1c5adeb1 and changes the hb_set page size to 8192. Attempt at keeping performance regressions in check without having to patch back the hb_set implementation to the pre 1.6.1 version. Bug: 782220 Change-Id: I9959d452fc944fd15092490beb45bbfc7f396d97 Tbr: kojii, eae, behdad Reviewed-on: https://chromium-review.googlesource.com/821546 Reviewed-by: Dominik Röttsches <drott@chromium.org> Commit-Queue: Dominik Röttsches <drott@chromium.org> Cr-Commit-Position: refs/heads/master@{#523421} [modify] https://crrev.com/e223d5aee70a70447a67910b7c8315c6aad777af/third_party/harfbuzz-ng/README.chromium [modify] https://crrev.com/e223d5aee70a70447a67910b7c8315c6aad777af/third_party/harfbuzz-ng/src/hb-set-private.hh [modify] https://crrev.com/e223d5aee70a70447a67910b7c8315c6aad777af/third_party/harfbuzz-ng/src/hb-set.cc
,
Dec 13 2017
This seems to work okay. The graphs for this issue are going up slightly again: https://chromeperf.appspot.com/group_report?bug_id=782220 - but without this change, i.e. without the new set implementation we start getting eqt_max, timeToFirstMeaningFulPaint regressions on linux-release, and win-high-dpi for FlipBoard, Dropbox, Elmundo, Economist, amazon.co.jp. See issues 794308 and 794309 .
,
Dec 15 2017
In issue 794896 we now found a more precise root cause, the Chandas system font has a huge GPOS table and gets used in some of the benchmarks, triggering delays when getting loaded into FreeType + HarfBuzz-assisted autohinting.
,
Jan 3 2018
Do we need to merge this for M64?
,
Jan 9 2018
Yes, I suggest to merge: https://chromium-review.googlesource.com/c/chromium/src/+/829378
,
Jan 9 2018
As confirmed over chat, this is a safe and tested merge. approving it.
,
Jan 9 2018
branch:3282
,
Jan 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6 commit 3ac50fb73d2d97b9a0870114a5d93e007ffa05b6 Author: Dominik Röttsches <drott@chromium.org> Date: Tue Jan 09 04:51:33 2018 [Merge] Do not perform per-character font fallback for PUA codepoints The analysis of a recent HarfBuzz performance regression showed that on Linux fontconfig may return obscure system fonts that have coverage in the Unicode private use area. Returning these has undefined semantics and is not what we want. This lookup may occur when icon fonts with PUA coverage are used as web fonts and have not been loaded yet. If we do not perform character fallback in these cases, the last resort font will be used instead. This speeds up timeToFirstPaint on pages which use PUA area icon fonts and where system fonts are installed which have coverage in this area - for example, it affects our browse:news:cnn, browse:news:hackernews and browse:social:twitter benchmark stories. TBR=drott@chromium.org (cherry picked from commit 725d3699e90b2ad35a7b9d139b890a55457d9a80) Bug: 795225 , 782220 Change-Id: Id5f87157aa7077cca8e1c0dc894bb76d2dd132c3 Reviewed-on: https://chromium-review.googlesource.com/829378 Commit-Queue: Dominik Röttsches <drott@chromium.org> Reviewed-by: Koji Ishii <kojii@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/855996 Reviewed-by: Dominik Röttsches <drott@chromium.org> Cr-Commit-Position: refs/branch-heads/3282@{#454} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [add] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/fallback-pua-last-resort-expected.txt [add] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/fallback-pua-last-resort.js [add] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/LayoutTests/platform/mac/inspector-protocol/layout-fonts/fallback-pua-last-resort-expected.txt [add] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/LayoutTests/platform/win7/inspector-protocol/layout-fonts/fallback-pua-last-resort-expected.txt [modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/fonts/FontCache.cpp [modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/fonts/FontCache.h [modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/fonts/FontCacheTest.cpp [modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/fonts/android/FontCacheAndroid.cpp [modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/fonts/fuchsia/FontCacheFuchsia.cpp [modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/fonts/linux/FontCacheLinux.cpp [modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm [modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/fonts/win/FontCacheSkiaWin.cpp [modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/text/Character.cpp [modify] https://crrev.com/3ac50fb73d2d97b9a0870114a5d93e007ffa05b6/third_party/WebKit/Source/platform/text/Character.h |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Nov 7 2017