New issue
Advanced search Search tips

Issue 781794 link

Starred by 1 user

Issue metadata

Status: Duplicate
Merged: issue 782220
Owner: ----
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression

Blocked on:
issue 786441



Sign in to add a comment

17.5%-562% regression in system_health.common_desktop at 513534:513616

Project Member Reported by mustaq@chromium.org, Nov 6 2017

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=781794

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


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

linux-release
Cc: drott@chromium.org bunge...@chromium.org e...@chromium.org
Owner: drott@chromium.org
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/125a3572f80000

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 4 by drott@chromium.org, Nov 7 2017

Owner: behdad@chromium.org
The benchmark regressions are in many cases in time to first paint / time to first contentful paint metrics. Could it be that HarfBuzz now always does some additional upfront analysis work that it didn't used to do, which is only relevant for the ligature search requests? For the benchmarks listed in #1, all system_health.common_desktop regressions are only on linux release, so I believe it is most likely something about the ligatures analysis steps for FT/HarfBuzz, and sadly, it has gotten slower than before?

We could experimentally disable #define FT_CONFIG_OPTION_USE_HARFBUZZ to see if this performance regression recovers even beyond the level of what it was before to see if this hypothesis is true.

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

Behdad, I am trying to get more insights by splitting all HarfBuzz changes we've applied between 1.5.1 and 1.6.3 in basically three groups. I've set up three perf_bisect runs:

https://chromium-review.googlesource.com/c/chromium/src/+/774880 HB 1.6.3 contains work on hb_set
https://chromium-review.googlesource.com/c/chromium/src/+/774879 Which is 1.6.0 + some changes before the set work started, plus the ICU 60 build fixes
https://chromium-review.googlesource.com/c/chromium/src/+/774878 Which is 1.5.1 plus ICU build fixes

These should run the following stories, each 5 times:
browse:social:twitter
browse:social:twitter_infinite_scroll
browse:news:cnn
browse:news:hackernews

And as we should check for
timeToFirstMeaningfulPaint, timeToFirstContentFulPaint, timeToFirstPaint 
 and total:500ms_window:renderer_eqt_max differences.

The linux_perf_bisect bot on each of these CLs is the relevant bot to look at, and the HTML results on these bots should be available from the bot logs.

This should be done latest in a few hours from now, would you take a look at these results, and see if that helps us get further?


Comment 6 by drott@chromium.org, Nov 16 2017

Also, could you explain what the benchmark you've been testing with is doing? I think these metrics here are not affected by raw shaping throughput, but rather by work that's done before the first character/glyph getting painted. I suspect it's mostly "setup/upfront-work costs" which goes into these.

Could we modify your benchmark in that direction? I.e. set up an hb_font/hb_face, get ready to shape, and either only get ready, or shape only one character or something, not necessarily even a ligature, then discard everything and set-up again, and repeat this a few hundred times?

Thanks!

Comment 7 by behdad@google.com, Nov 17 2017

My benchmark was loading the font using FreeType and then just loading one glyph.  Repeating this a thousand times.  I'll check again tomorrow, don't have my laptop tonight.

How do the bisected perf results look?
It's possible to see the full results for these by going to the above CLs, clicking on the linux_perf_bisect bot, then clicking on "HTML Results". The short summary is that the bot seems to think all of these are a bit faster than tip of tree, but to varying amounts.

https://chromium-review.googlesource.com/c/chromium/src/+/774880
Not very significant.

https://chromium-review.googlesource.com/c/chromium/src/+/774879
cpuTimeToFirstMeaningfulPaint and total:500ms_window:renderer_eqt_cpu are significantly faster with the patch than at current tip of tree.

https://chromium-review.googlesource.com/c/chromium/src/+/774878
cpuTimeToFirstMeaningfulPaint, timeToFirstContentfulPaint, timeToFirstMeaningfulPaint, timeToFirstPaint, total:500ms_window:renderer_eqt, and total:500ms_window:renderer_eqt_cpu are significantly faster with the patch than at current tip of tree.

Comment 9 by drott@chromium.org, Nov 17 2017

I started two more:
https://chromium-review.googlesource.com/c/chromium/src/+/776879 
harfBuzzPerfBisect_noOp_control - same as TOT, except no call to set_ptem

https://chromium-review.googlesource.com/c/chromium/src/+/776783
change only README, set ptem

I am awaiting these results to have some controls / verification. I was surprised to see that 
https://chromium-review.googlesource.com/c/chromium/src/+/774880 actually did have differences to trunk. The real functional difference there was only not calling hb_font_set_ptem.

Comment 10 by drott@chromium.org, Nov 17 2017

Blockedon: 786441
Cc: primiano@chromium.org
Components: Speed>Telemetry Speed>Dashboard
Owner: ----
Status: Available (was: Assigned)
I've filed  issue 786441  since even a no-op CL only touching a README produces 24% regressions on the timeToFirst...Paint metrics. Given these circumstance, I am doing either doing something wrong in the way I set up these perf bisects, or the tooling has some significant bugs.

Primiano, can you CC people who can help with analysing what's really going on here? The HarfBuzz roll in question has important correctness fixes and we need to get a clearer picture if we really have a performance issue here or not.

As mentioned in the new  issue 786441  I ran the bisects using the following command:
tools/perf/run_benchmark try linux system_health.common_desktop  --pageset-repeat=5  --story-filter='(.*browse.*social.*twitter.*|.*browse.*news*.cnn.*|.*browse.*news.*hackernews.*)'

Setting to Available for triaging in Speed>Dashboard & Speed>Telemetry.


Cc: dtu@chromium.org
>a no-op CL only touching a README produces 24% regressions on the timeToFirst...Paint metrics

+Dave/Simon: this seems liek a problem with the perf bisect. 
I've commented the blocking bug  crbug.com/786441 

Comment 13 by js...@chromium.org, Nov 22 2017

Cc: js...@chromium.org

Comment 14 by drott@chromium.org, Nov 23 2017

In https://chromium-review.googlesource.com/c/chromium/src/+/788393 I've made a simple HarfBuzz performance test and ran this against the Chrome based on 1.5.1 plus ICU fixes and against 1.6.3.

With this relatively simple test I can't detect any performance changes.

Behdad, could you take a look a the benchmark and sanity check it?

"sans-serif"

1.5.1 ICUFixes
[----------] 1 test from HarfBuzzShaperTest (13238 ms total)
[----------] 1 test from HarfBuzzShaperTest (13739 ms total)
[----------] 1 test from HarfBuzzShaperTest (13319 ms total)
[----------] 1 test from HarfBuzzShaperTest (14409 ms total)
[----------] 1 test from HarfBuzzShaperTest (13265 ms total)

1.6.3 branch
[----------] 1 test from HarfBuzzShaperTest (14012 ms total)
[----------] 1 test from HarfBuzzShaperTest (13707 ms total)
[----------] 1 test from HarfBuzzShaperTest (13335 ms total)
[----------] 1 test from HarfBuzzShaperTest (13346 ms total)
[----------] 1 test from HarfBuzzShaperTest (13878 ms total)


ROBOTO

1.5.1 ICU fixes
[==========] 1 test from 1 test case ran. (17377 ms total)
[==========] 1 test from 1 test case ran. (17613 ms total)
[==========] 1 test from 1 test case ran. (18169 ms total)
[==========] 1 test from 1 test case ran. (18092 ms total)
[==========] 1 test from 1 test case ran. (18190 ms total)


1.6.3 branch
[==========] 1 test from 1 test case ran. (17447 ms total)
[==========] 1 test from 1 test case ran. (18356 ms total)
[==========] 1 test from 1 test case ran. (18178 ms total)
[==========] 1 test from 1 test case ran. (17725 ms total)
[==========] 1 test from 1 test case ran. (17608 ms total)


Project Member

Comment 15 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

Mergedinto: 782220
Status: Duplicate (was: Available)

Comment 17 by benhenry@google.com, Jan 16 (6 days ago)

Components: Test>Telemetry

Comment 18 by benhenry@google.com, Jan 16 (6 days ago)

Components: -Speed>Telemetry

Sign in to add a comment