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

Issue 731659 link

Starred by 3 users
Status: WontFix
Owner:
█ cathiec...@tencent.com
Last visit > 30 days ago
Closed: Jul 2017
Cc:
pdr@chromium.org
█ cathiec...@tencent.com
█ alexclarke@chromium.org
skobes@chromium.org
primiano@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

22.3%-22.6% regression in blink_perf.dom at 477086:477215

Project Member Reported by primiano@chromium.org, Jun 9 2017 
See the link to graphs below.
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Jun 9 2017

Cc: cathiec...@tencent.com
Owner: cathiec...@tencent.com

=== Auto-CCing suspected CL author cathiechen@tencent.com ===

Hi cathiechen@tencent.com, 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 : cathiechen
  Commit : 47f9b0ff7d4594f6633956ac570966439b4d5125
  Date   : Tue Jun 06 04:51:41 2017
  Subject: Fix font-size shaking issue in some page

Bisect Details
  Configuration: android_one_perf_bisect
  Benchmark    : blink_perf.dom
  Metric       : select-long-word/select-long-word
  Change       : 22.83% | 2221.3325 -> 2728.52216667

Revision             Result                  N
chromium@477085      2221.33 +- 34.4353      6      good
chromium@477142      2218.5 +- 45.4762       6      good
chromium@477171      2227.48 +- 13.6182      6      good
chromium@477185      2219.64 +- 22.8399      6      good
chromium@477192      2215.02 +- 27.1953      6      good
chromium@477196      2229.69 +- 11.5384      6      good
chromium@477197      2226.78 +- 20.7106      6      good
chromium@477198      2719.59 +- 22.214       6      bad       <--
chromium@477199      2728.52 +- 28.3465      6      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.dom

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8977268060866967760

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5773326762377216


| 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 Speed>Bisection.  Thank you!
Project Member

Comment 4 by 42576172...@developer.gserviceaccount.com, Jun 10 2017 

 Issue 731681  has been merged into this issue.

Comment 5 by cathiec...@tencent.com, Jun 12 2017

Status: Started (was: Untriaged)
Working on this.
Project Member

Comment 6 by 42576172...@developer.gserviceaccount.com, Jun 12 2017

Cc: alexclarke@chromium.org
 Issue 732311  has been merged into this issue.
Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, Jun 15 2017 

 Issue 732309  has been merged into this issue.

Comment 8 by cathiec...@tencent.com, Jul 5 2017 

Sorry for the delay.

Here is the summary of these issues.

In order to prevent the shaking issue, this CL make root cluster use LayoutView as WidthProvider that might enlarge font size a bit on some page.

1.  Issue 731681 : multicol_lots-of-text-balanced, fixed. For more details https://codereview.chromium.org/2961583003

2.  Issue 731659 : select-long-word and  Issue 732309 : tough_scrolling_cases, won't fix. These regressions are caused by font size enlarging.

  2.1 select-long-word:
    Before this CL root cluster's WidthProvider is 800px(<div id="long">). After CL it's changed to 980px(LayoutView). So the font size multiplier change from 2.5 to 3.0625.
    'select-long-word' tests the performance of `getSelection().modify("extend", "forward", "word")`. Finding the end position of word cost the most time.
    According to the procedure of VisibleUnits.cpp NextBoundaryAlgorithm(), it will "Keep asking the iterator for chunks until the search function returns an end value not equal to the length of the string passed to it." These chunks are generated by inlineTextBox. As the font size enlarged, it would generate more inlineTextBox. That's the reason of this regression.

  2.2 tough_scrolling_cases:
    Before this CL root cluster's WidthProvider is 964px(<body>), after it WidthProvider is 980px(LayoutView). Multiplier change from 3.0125 to 3.0625.
    According to [scroll.js](https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/actions/scroll.js)  ScrollAction.prototype.startGesture_

        var max_scroll_length_pixels = (MAX_SCROLL_LENGTH_TIME_MS / 1000) * this.options_.speed_;
        var distance = Math.min(max_scroll_length_pixels, this.getScrollDistance_());

    The scroll `distance` might be `this.getScrollDistance_()` if speed_ is big enough.

        ScrollAction.prototype.getScrollDistanceDown_ = function() {
          var clientHeight;
          // clientHeight is "special" for the body element.
          if (this.element_ == document.body)
            clientHeight = __GestureCommon_GetWindowHeight();
          else
            clientHeight = this.element_.clientHeight;
          return this.element_.scrollHeight -
                 this.element_.scrollTop -
                 clientHeight;
        };

    `this.element` is <HTML>. For the font size enlarged, the distance will bigger than before. That's the reason of regression.

3.  Issue 732311 : html5-full-render, couldn't reproduce.
  Couldn't reproduce this regression at my local nexus7 telemetry environment. alexclarke@ could you check it again?

Comment 9 by cathiec...@tencent.com, Jul 5 2017

Cc: pdr@chromium.org skobes@chromium.org
Project Member

Comment 11 by 42576172...@developer.gserviceaccount.com, Jul 6 2017 


=== BISECT JOB RESULTS ===
NO Perf regression found

Bisect Details
  Configuration: android_nexus7_perf_bisect
  Benchmark    : blink_perf.parser
  Metric       : html5-full-render/html5-full-render

Revision             Result                  N
chromium@477118      36310.3 +- 486.196      21      good
chromium@477197      36254.1 +- 516.909      21      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.parser

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8974850966071636704


For feedback, file a bug with component Speed>Bisection

Comment 12 by cathiec...@tencent.com, Jul 12 2017 

Update:

 Issue 732311  : `html5-full-render`, could reproduce.

Before this CL root cluster's WidthProvider is 828px(<body> with padding:152px). After CL it's changed to 980px(LayoutView). So the font size multiplier change from 2.5875 to 3.0625.

`html5-full-render` tests the layout time. Enlarge font-size and generate more inlineTextBox will cost more layout time too.

In a word,  Issue 732311 ,  Issue 731659  and  Issue 732309  are sensitive to font-size.

Comment 13 by skobes@chromium.org, Jul 12 2017

Status: WontFix (was: Started)
Closing this bug as WontFix based on the analysis in comment #8 and the discussion on http://crrev.com/c/566762.

The reported regressions appear to be a side effect of having more lines / larger scroll dimensions in the test case after increasing the text autosizing multiplier.  Therefore they are not likely to reflect any real-world performance impact.
►

Sign in to add a comment