New issue
Advanced search Search tips

Issue 631034 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Jul 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

50.7% regression in page_cycler.intl_ar_fa_he at 407176:407206

Project Member Reported by primiano@chromium.org, Jul 25 2016

Issue description

Somehow I suspect

https://codereview.chromium.org/2148363004

Let's see what bisect says
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=631034

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg2uK-rwoM


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

chromium-rel-mac-retina
Project Member

Comment 4 by 42576172...@developer.gserviceaccount.com, Jul 25 2016

Cc: drott@chromium.org
Owner: drott@chromium.org

=== Auto-CCing suspected CL author drott@chromium.org ===

Hi drott@chromium.org, the bisect results pointed to your CL below as possibly
causing a regression. Please have a look at this info and see whether
your CL be related.


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Fix infinite recursion crash in HarfBuzz' CoreText backend
Author  : drott
Commit description:
  
The font cascade reconfiguration which was introduced as fix for AAT
shaping performance regressions in  crbug.com/547912  seems to
occasionally cause CoreText crashes on OS X 10.9. We don't have a better
way of detecting this than by OS or CoreText API version number. This is
one of our top Mac crashers on Mac OS 10.9 with Chrome across versions [1].

This crash does not occur in newer versions of OS X and we can keep this
important performance optimization enabled there.

A big thanks to Robert Sesek (rsesek@) for the patient and thorough
initial investigation. Discussing and working together on this issue we
were able to identify the crash triggering code in HarfBuzz in this
case.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=576941#c74

BUG= 576941 ,  625902 

Review-Url: https://codereview.chromium.org/2173883002
Cr-Commit-Position: refs/heads/master@{#407185}
Commit  : 64a2d4d02ea769c849df4718d196df12a3f79091
Date    : Fri Jul 22 17:18:48 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@407175  621.952  2.74092  5  good
chromium@407183  624.967  4.69302  5  good
chromium@407184  630.524  9.82207  5  good
chromium@407185  951.818  4.01811  5  bad    <--
chromium@407187  945.421  9.26932  5  bad
chromium@407191  950.958  4.91493  5  bad
chromium@407206  947.781  5.24399  5  bad

Bisect job ran on: mac_retina_perf_bisect
Bug ID: 631034

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests page_cycler.intl_ar_fa_he
Test Metric: warm_times/page_load_time
Relative Change: 52.39%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_retina_perf_bisect/builds/1439
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9006155880182869568


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=4565953010991104

| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Tests>AutoBisect.  Thank you!
Project Member

Comment 5 by 42576172...@developer.gserviceaccount.com, Jul 25 2016


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Fix infinite recursion crash in HarfBuzz' CoreText backend
Author  : drott
Commit description:
  
The font cascade reconfiguration which was introduced as fix for AAT
shaping performance regressions in  crbug.com/547912  seems to
occasionally cause CoreText crashes on OS X 10.9. We don't have a better
way of detecting this than by OS or CoreText API version number. This is
one of our top Mac crashers on Mac OS 10.9 with Chrome across versions [1].

This crash does not occur in newer versions of OS X and we can keep this
important performance optimization enabled there.

A big thanks to Robert Sesek (rsesek@) for the patient and thorough
initial investigation. Discussing and working together on this issue we
were able to identify the crash triggering code in HarfBuzz in this
case.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=576941#c74

BUG= 576941 ,  625902 

Review-Url: https://codereview.chromium.org/2173883002
Cr-Commit-Position: refs/heads/master@{#407185}
Commit  : 64a2d4d02ea769c849df4718d196df12a3f79091
Date    : Fri Jul 22 17:18:48 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@407175  621.747  3.12707  5  good
chromium@407183  623.92   5.02008  5  good
chromium@407184  629.403  5.536    5  good
chromium@407185  953.529  4.10602  5  bad    <--
chromium@407187  950.653  6.57863  5  bad
chromium@407191  942.525  9.69588  5  bad
chromium@407206  939.52   6.46958  5  bad

Bisect job ran on: mac_retina_perf_bisect
Bug ID: 631034

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests page_cycler.intl_ar_fa_he
Test Metric: warm_times/page_load_time
Relative Change: 51.11%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_retina_perf_bisect/builds/1438
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9006155890671272112


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=6299567704768512

| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Tests>AutoBisect.  Thank you!

Comment 6 by drott@chromium.org, Jul 25 2016

Cc: rsesek@chromium.org primiano@chromium.org
Status: WontFix (was: Assigned)
Unfortunately, I'll mark this as WontFix, this was an expected regression on OS X 10.9 - 10.10 and later versions do not have this problem, but as you can see from the bugs in the CL description we run into CoreText crashes without this fix.

Maybe silly proposal of the form "why don't you just".
I imagine the problem there is that CTGetCoreTextVersion() is being called in a fastpath. As the retval is a uint32_t can't you cache that in a function-static field and do the call to CTGetCoreTextVersion() only once? I imagine that one can assume that the coretext version doesn't change during the lifetime of a chrome execution, right?

Comment 8 by rsesek@chromium.org, Jul 25 2016

CTGetCoreTextVersion is fast, though there is of course overhead with a function call. It's more likely the regression is from not using the caching optimization that was causing the crashes.

                     _CTGetCoreTextVersion:
00000000000d0758         push       rbp
00000000000d0759         mov        rbp, rsp
00000000000d075c         mov        eax, 0x80000
00000000000d0761         pop        rbp
00000000000d0762         ret        
                        ; endp
00000000000d0763         nop        
Ah ok. nvm then, I misread the intent of the code. I thought we were crashing immediately if not hitting that if, instead, reading more thoroughly, we were crashing inside CoreText from time to time.
I suppose there is nothing really doable here. I take back my "why don't we just" (I told you that it was silly :P)

Sign in to add a comment