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

Issue 794896 link

Starred by 2 users

Issue metadata

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

Blocked on:
issue 795225
issue 796124



Sign in to add a comment

4.4%-183% regression in system_health.common_desktop at 523386:523431

Project Member Reported by kraynov@chromium.org, Dec 14 2017

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Dec 14 2017

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

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


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

chromium-rel-mac11-pro
linux-release
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Dec 14 2017

Cc: drott@chromium.org
Owner: drott@chromium.org
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/11a95132040000

Revert patching hb_set back to old impl, but increase page size
By drott@chromium.org · Tue Dec 12 13:37:26 2017
chromium @ e223d5aee70a70447a67910b7c8315c6aad777af

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Comment 4 by drott@chromium.org, Dec 14 2017

Cc: behdad@chromium.org
This is the continuation of 782220, likely a WontFix. We gain performance on other platforms with the hb_set changes. Due to the interactions between HarfBuzz and FreeType for autohinting, we see impact on some of the benchmark sites on Linux.

We can try to collect the fonts that are used in those benchmarks and see if they contain particular hard cases for autohinting and try to tune hb_set page size further.

Comment 5 by drott@chromium.org, Dec 14 2017

Behdad, for browse_news_cnn, browse_news_hackernews and browse_social_twitter I deduplicated and collected the fonts at the shaping stage [1] and dumped them using their family name, SkTypeface::uniqueId() (which hints at the order in which they were used), and the full blob (these include system fonts as well as web fonts, without distinction.)

The ZIP including all those is here, shared with you:
https://drive.google.com/file/d/1dJO3S1IwZBITcN5lYY-Im94ZxH2qxp7Z/view?usp=sharing

I'm attaching 'benchmark_stories_font_list.txt' which contains the list of found fonts. 

Can you take a look, and perhaps benchmark those against the hb_set changes? Do you see any patterns?

FWIW, what I find slightly curious is that we get Chandas in all cases, which I believe is a Devanagari font. Perhaps fontconfig decides to return this as font fallback for something.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/827070/1/third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp
benchmark_stories_font_list.txt
2.9 KB View Download

Comment 6 by behdad@chromium.org, Dec 14 2017

Just loading Chandas using FreeType with HB enabled takes half a second with pagesize=512!!!! And yes, with old set, it was five times faster!

We've found the culprit. Let me see what's going on. Something *very* curious going on. Even with a pagesize of 64k, old impl is four times faster in both of my benchmarks. That's so weird but imaginable.

The slowdown is expected, given that new set does more work in add(). The real problem is that add() is called 50M times in this font.  I believe I can adjust my algorithms somewhere to do significantly less work.

Oh and the font is definitely unique in that it has a 600KB GPOS table.
I've never ever seen anything like that.

To be continued...

Comment 7 by drott@chromium.org, Dec 15 2017

Cc: bunge...@chromium.org

Comment 8 by behdad@chromium.org, Dec 16 2017

So, basically, Chandas is full of garbage...  I've made HarfBuzz detect this and bail out.  Will push that to master soon. At some point I like to add a work counter to collect_glyphs() to protect against malicious fonts as well.

Comment 9 by behdad@chromium.org, Dec 16 2017

HarfBuzz changes are in master.  I'll roll a release soon; though, would be good to get some testing.

Comment 10 by drott@chromium.org, Dec 19 2017

Cc: mlippautz@chromium.org
 Issue 796132  has been merged into this issue.

Comment 11 by drott@chromium.org, Dec 19 2017

Blockedon: 796124 795225
Project Member

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

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

commit a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1
Author: Dominik Röttsches <drott@chromium.org>
Date: Wed Dec 20 13:26:18 2017

Roll HarfBuzz to 1.7.3 plus hb_set fix

Roll HarfBuzz to 1.7.3 to incorporate hb_set changes which improve
dealing with fonts containing incorrectly sorted range information, such
as the Chandas Devanagari font part of the ttf-devanagari-fonts package,
as identified in
https://bugs.chromium.org/p/chromium/issues/detail?id=794896

Cherry-pick 2fe5f885..a9432bde on top of 1.7.3 to incorporate hb_set
regression fixes.

Bug:  794896 ,  796124 
Change-Id: I9256018b6ec6c7e84f9261c97758b2bee2d410a2
Tbr: kojii, eae, behdad
Reviewed-on: https://chromium-review.googlesource.com/831946
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525317}
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/NEWS
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/README.chromium
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-blob.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-buffer-deserialize-json.hh
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-buffer-deserialize-text.hh
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-buffer.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-common.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-coretext.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-coretext.h
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-debug.hh
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-ft.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-glib.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-graphite2.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-icu.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-ot-font.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-ot-layout-common-private.hh
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-ot-layout-gpos-table.hh
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-ot-layout-gsub-table.hh
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-ot-layout.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-ot-shape-complex-arabic-fallback.hh
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-ot-shape-complex-arabic.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-ot-shape-complex-indic.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-ot-shape-complex-use-table.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-ot-shape-fallback.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-ot-shape.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-ot-tag.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-private.hh
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-set-digest-private.hh
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-set-private.hh
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-set.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-shape-plan.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/hb-ucdn.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/main.cc
[modify] https://crrev.com/a14e00ae9bc6a9263bbfb249ec51b4ed6a7635c1/third_party/harfbuzz-ng/src/test-buffer-serialize.cc

Comment 13 by drott@chromium.org, Dec 21 2017

Status: Fixed (was: Assigned)
Yes, these hb_set improvements and the measures to reject incorrectly sorted data in Chandas did help and brought the graphs from  issue 782220  back to normal: https://chromeperf.appspot.com/group_report?bug_id=782220 

Sign in to add a comment