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

Issue 862352 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 20
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

1.4%-9.5% regression in system_health.memory_mobile at 1530634860:1530651231

Project Member Reported by tdres...@chromium.org, Jul 10

Issue description

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

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


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

health-plan-clankium-phone
health-plan-webview-low-end-phone
health-plan-webview-phone
perf-go-phone-1024
perf-go-webview-phone
Cc: drott@chromium.org
Owner: drott@chromium.org
Status: Assigned (was: Untriaged)

=== 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
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?
Cc: nednguyen@chromium.org
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?
Cc: perezju@chromium.org
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
 Issue 862353  has been merged into this issue.
Thanks for the additional pointers, I'll try with that.


=== 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
Status: Started (was: Assigned)
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

Wow, that's great! Thanks for the diligence in investigating this regression and getting a fix!
Project Member

Comment 13 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Those of the graphs have recovered where a significant increase after r572159 was observed, marking this as fixed.

Amazing. Thanks for taking time to address the regression!

Sign in to add a comment