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

Issue 782220 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug-Regression



Sign in to add a comment

84.7%-559.4% regression in v8.browsing_desktop at 513534:513616

Project Member Reported by mvstanton@google.com, Nov 7 2017

Issue description

STrange. Must bisect this one.

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

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


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

linux-release
Project Member

Comment 4 by 42576172...@developer.gserviceaccount.com, Nov 21 2017

Cc: drott@chromium.org bunge...@chromium.org e...@chromium.org
Owner: drott@chromium.org
📍 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

Comment 5 by drott@chromium.org, Nov 21 2017

Cc: behdad@chromium.org
Probably a duplicate of  issue 781794  - mvstanton@, what are these renderer_eqt benchmarks? What do they compute?
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. 

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. 
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. 

Comment 11 by drott@chromium.org, 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.

Project Member

Comment 12 by 42576172...@developer.gserviceaccount.com, 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
Project Member

Comment 13 by 42576172...@developer.gserviceaccount.com, 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

Comment 15 by drott@chromium.org, 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

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?

Comment 17 by behdad@google.com, 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
Project Member

Comment 18 by bugdroid1@chromium.org, 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

Cc: primiano@chromium.org mustaq@chromium.org dtu@chromium.org js...@chromium.org
 Issue 781794  has been merged into this issue.
Status: Fixed (was: Assigned)
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.


Labels: Merge-Request-64
Labels: OS-Linux
Project Member

Comment 24 by sheriffbot@chromium.org, Dec 5 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
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. 
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.
Project Member

Comment 28 by bugdroid1@chromium.org, 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

Cc: petermarshall@chromium.org kojii@chromium.org
 Issue 792891  has been merged into this issue.
Project Member

Comment 30 by sheriffbot@chromium.org, 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
Project Member

Comment 31 by bugdroid1@chromium.org, 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

Comment 32 by drott@chromium.org, 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 .



Comment 33 by drott@chromium.org, 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.

Do we need to merge this for M64?
Labels: -Merge-Approved-64
As confirmed over chat, this is a safe and tested merge. approving it.
branch:3282
Project Member

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

Labels: merge-merged-3282
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