Issue metadata
Sign in to add a comment
|
50.7% regression in page_cycler.intl_ar_fa_he at 407176:407206 |
||||||||||||||||||||
Issue descriptionSomehow I suspect https://codereview.chromium.org/2148363004 Let's see what bisect says
,
Jul 25 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/9006155890671272112
,
Jul 25 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/9006155880182869568
,
Jul 25 2016
=== 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!
,
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!
,
Jul 25 2016
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.
,
Jul 25 2016
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?
,
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
,
Jul 26 2016
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 |
|||||||||||||||||||||
Comment 1 by primiano@chromium.org
, Jul 25 2016