Issue metadata
Sign in to add a comment
|
1.4%-9.5% regression in system_health.memory_mobile at 1530634860:1530651231 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jul 10
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8941358149986669808
,
Jul 11
=== Auto-CCing suspected CL author drott@chromium.org === Hi drott@chromium.org, the bisect results pointed to your CL, please take a look at the results. === BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : Dominik Röttsches Commit : 42eb6fbdb9e6783542d49235322aa92a4b4aebc9 Date : Tue Jul 03 09:40:13 2018 Subject: Don't exclude unassigned characters from fallback Bisect Details Configuration: go-webview-phone-perf-bisect Benchmark : system_health.memory_mobile Metric : memory:webview:all_processes:reported_by_os:system_memory:native_heap:proportional_resident_size_avg/browse_shopping/browse_shopping_amazon Change : 9.73% | 34349570.6667 -> 37691736.0 Revision Result N android-chrome@25f69d9f4c 34349571 +- 501718 6 good android-chrome@25f69d9f4c,chromium@572159 37572611 +- 663166 6 bad <-- android-chrome@25f69d9f4c,chromium@572160 37434712 +- 151207 6 bad android-chrome@25f69d9f4c,chromium@572162 37579437 +- 371922 6 bad android-chrome@25f69d9f4c,chromium@572165 37371224 +- 410406 6 bad android-chrome@77aab0d246 37457923 +- 152459 6 bad android-chrome@6e2ac71aeb 37691736 +- 741258 6 bad Please refer to the following doc on diagnosing memory regressions: https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md To Run This Test src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=browse.shopping.amazon system_health.memory_mobile More information on addressing performance regressions: http://g.co/ChromePerformanceRegressions Debug information about this bisect: https://chromeperf.appspot.com/buildbucket_job_status/8941358149986669808 For feedback, file a bug with component Speed>Bisection
,
Jul 11
This is odd, my CL allows performing fallback for Unicode unassigned characters, in other words invalid characters. If there is a difference before and after, that would mean that this particular Amazon benchmark page triggers fallback for unassigned characters and perhaps loads an additional font because of that. Since I don't have access to an Android device at the moment, is it possible to open this benchmark page on desktop to have a look whether it reaches the unsassigned-fallback code?
,
Jul 11
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8941297108821673552
,
Jul 11
Kicked off another bisect, to make sure this wasn't just a bad bisect. To view this page on desktop, I suspect you could coax WebPageReplay into serving up the page, and then you could load it with devtools mobile emulation. Ned, can you provide some pointers here?
,
Jul 11
To run this on desktop: 1) Remove the SUPPORTED_PLATFORMS = [story.expectations.ALL_MOBILE] in https://cs.chromium.org/chromium/src/tools/perf/benchmarks/system_health.py?rcl=40aa3c8062163cc416593a354c8f8d41da37fe8e&l=133 2) Run ./tools/perf/run_benchmark system_health.memory_mobile --story-filter=browse:shopping:amazon
,
Jul 11
Issue 862353 has been merged into this issue.
,
Jul 11
Thanks for the additional pointers, I'll try with that.
,
Jul 12
=== BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : Dominik Röttsches Commit : 42eb6fbdb9e6783542d49235322aa92a4b4aebc9 Date : Tue Jul 03 09:40:13 2018 Subject: Don't exclude unassigned characters from fallback Bisect Details Configuration: go-webview-phone-perf-bisect Benchmark : system_health.memory_mobile Metric : memory:webview:all_processes:reported_by_os:system_memory:native_heap:proportional_resident_size_avg/browse_shopping/browse_shopping_amazon Change : 8.95% | 34617688.0 -> 37715970.6667 Revision Result N android-chrome@25f69d9f4c 34617688 +- 458874 6 good android-chrome@25f69d9f4c,chromium@572159 37645997 +- 468502 6 bad <-- android-chrome@25f69d9f4c,chromium@572160 37566125 +- 758157 6 bad android-chrome@25f69d9f4c,chromium@572162 37340504 +- 181704 6 bad android-chrome@25f69d9f4c,chromium@572165 37307053 +- 411754 6 bad android-chrome@77aab0d246 37738499 +- 724089 6 bad android-chrome@6e2ac71aeb 37715971 +- 775084 6 bad Please refer to the following doc on diagnosing memory regressions: https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md To Run This Test src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=browse.shopping.amazon system_health.memory_mobile More information on addressing performance regressions: http://g.co/ChromePerformanceRegressions Debug information about this bisect: https://chromeperf.appspot.com/buildbucket_job_status/8941297108821673552 For feedback, file a bug with component Speed>Bisection
,
Jul 19
Re #7, nednguyen@, thanks, this was very helpful. We're benchmarking Amazon.in which does perform fallback for U+FFFE, a sentinel character from the non-character set of characters, which we started allowing again after my CL. Fix CL up for review in https://chromium-review.googlesource.com/c/chromium/src/+/1143277
,
Jul 19
Wow, that's great! Thanks for the diligence in investigating this regression and getting a fix!
,
Jul 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8b3269e492d0e6027ae9d197235b69d8dc4bdead commit 8b3269e492d0e6027ae9d197235b69d8dc4bdead Author: Dominik Röttsches <drott@chromium.org> Date: Thu Jul 19 15:16:18 2018 Do not perform fallback for non-characters In r492524 we resumed performing per-character fallback for unassigned characters. This was intended to avoid problems upgrading the emoji data file. When updating the emoji data before updating ICU new emoji may have appeared in the unassigned range, which we would erroneously exclude from fallback in that case. However, allowing the full range of unassigned characters does not make sense either and triggered a memory regression in our benchmark when the amazon.in page triggers fallback for the non printable U+FFFE sentinel character. To solve this, in addition to private use area codepoints we exclude 66 Unicode non-characters, defined in http://www.unicode.org/faq/private_use.html#nonchar4 They are part of the unassigned range, but have special meanings and are explicitly intended as non-characters. We must not try to look for glyphs in fonts for these. Bug: 862352 Change-Id: I50b22a4acb0d148595c5ae4f99a57d59ee8c41dd Reviewed-on: https://chromium-review.googlesource.com/1143277 Reviewed-by: Koji Ishii <kojii@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Commit-Queue: Dominik Röttsches <drott@chromium.org> Cr-Commit-Position: refs/heads/master@{#576499} [modify] https://crrev.com/8b3269e492d0e6027ae9d197235b69d8dc4bdead/third_party/blink/renderer/platform/fonts/font_cache.cc [modify] https://crrev.com/8b3269e492d0e6027ae9d197235b69d8dc4bdead/third_party/blink/renderer/platform/text/character.cc [modify] https://crrev.com/8b3269e492d0e6027ae9d197235b69d8dc4bdead/third_party/blink/renderer/platform/text/character.h [modify] https://crrev.com/8b3269e492d0e6027ae9d197235b69d8dc4bdead/third_party/blink/renderer/platform/text/character_test.cc
,
Jul 20
Those of the graphs have recovered where a significant increase after r572159 was observed, marking this as fixed.
,
Jul 20
Amazing. Thanks for taking time to address the regression! |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Jul 10