All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=798369

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


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

android-nexus5X

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8958513240458935280

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

Hi jaebaek@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 : Jaebaek Seo
  Commit : 1a859d302e47f3b8c2376c0a7bcfbf9e9dda6287
  Date   : Tue Dec 26 03:12:37 2017
  Subject: Scale autosized font by DSF when --use-zoom-for-dsf is enabled

Bisect Details
  Configuration: android_nexus5X_perf_bisect
  Benchmark    : blink_perf.paint
  Metric       : color-changes/color-changes
  Change       : 12.43% | 261.279055556 -> 293.760833333

Revision             Result                  N
chromium@526183      261.279 +- 49.337       9      good
chromium@526184      283.518 +- 3.99384      9      bad       <--
chromium@526185      292.969 +- 85.6792      9      bad
chromium@526186      286.544 +- 29.9684      9      bad
chromium@526188      293.761 +- 91.1404      9      bad

Please refer to the following doc on diagnosing blink_perf regressions:
  https://chromium.googlesource.com/chromium/src/+/master/docs/speed/benchmark_harnesses/blink_perf.md

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.paint

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

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


For feedback, file a bug with component Speed>Bisection

Hmm .. ?
alexclarke@, are you sure this performance issue is from my CL?

I just changes 3 line in Blink and as far as I checked, it must have a negligible performance degradation.

The reasoning is here:
The only case that it has a big impact on performance is that it triggers re-layout because of the font size update. As far as I checked, there is no re-layout. Even if I had a mistake, it still does not make sense because re-layout take lots of time rather than 10.1%.

Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8958427953709004928

The /bot/ thought it was your patch, I'm less certain, there doesn't seem to be a sustained regression. Lets see if the second bisect finds anything.

=== BISECT JOB RESULTS ===
Perf regression found with culprit

Suspected Commit
  Author : Jaebaek Seo
  Commit : 1a859d302e47f3b8c2376c0a7bcfbf9e9dda6287
  Date   : Tue Dec 26 03:12:37 2017
  Subject: Scale autosized font by DSF when --use-zoom-for-dsf is enabled

Bisect Details
  Configuration: android_nexus5X_perf_bisect
  Benchmark    : blink_perf.paint
  Metric       : color-changes/color-changes
  Change       : 10.62% | 256.5275 -> 283.758833333

Revision             Result                  N
chromium@526183      256.527 +- 4.88554      6      good
chromium@526184      285.011 +- 2.24427      6      bad       <--
chromium@526185      283.304 +- 2.31529      6      bad
chromium@526186      284.287 +- 2.20405      6      bad
chromium@526188      283.759 +- 1.37897      6      bad

Please refer to the following doc on diagnosing blink_perf regressions:
  https://chromium.googlesource.com/chromium/src/+/master/docs/speed/benchmark_harnesses/blink_perf.md

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.paint

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

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


For feedback, file a bug with component Speed>Bisection

The bot found your patch again.  It seems your patch has unexpected side effects, could you please investigate.

Ok, thank you for doublechecking it. I will take a look.

+ skobes@

skobes@, could you give me any clues related to this bug?
The perf regression bot reported that crrev.com/c/831814 caused 10% performance degradation. As far as I checked the code, it does not trigger re-layout.

Are you able to reproduce it locally?

No .. I tried the following command but it stopped (I don't know why).

$ tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests blink_perf.paint

I will try it again, but I think I must analyze it using trace (https://www.chromium.org/developers/how-tos/trace-event-profiling-tool/recording-tracing-runs).

If you know anything that potentially degrades the performance (because of the zoom for the text-autosizing font maybe?), please let me know it.

In some cases, autosizing content that previously wasn't autosized can regress a perf benchmark due to the cost of laying out more lines of text (e.g.  issue 675534 ).  But this could be something entirely different, I don't know.

Attachments are test results from my local machine.
(I ran them by hitting this command

$ ./tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --pageset-repeat=1 --also-run-disabled-tests blink_perf --test-path=Paint/color-changes.html
)

results-chart-without-patch.json is the result running without my patch and
results-chart-with-patch.json is the result running with my patch.

(I manually removed the update shown in
https://chromium.googlesource.com/chromium/src/+/1a859d302e47f3b8c2376c0a7bcfbf9e9dda6287
)

In this local experiment, my patch does not cause performance problem ..
I get averages of those numbers and those three tests for color-changes.html (i.e., LocalFrameView::paintTree, LocalFrameView::prePaint, and just color-changes.html) shows a better performance.

alexclarke@, I am sorry but could you please check the problem again?

Deleted: results-chart-without-patch.json 4.0 KB

results-chart-without-patch.json 4.0 KB View Download

Deleted: results-chart-with-patch.json 3.9 KB

results-chart-with-patch.json 3.9 KB View Download

I tested it once more to be clear and realized the fluctuation of the performance is not small .. I just guess the test script does not kill background running processes (nor fix the priority of tested process).

See the attachments. I just did the same experiment, but performance degraded a little bit when I applied my patch (1.7 ~ 1.8 % for average values).

If the bug report just stems from factors causing the unexpected fluctuation, what should I do?
Does the performance testing bot remove those factors?

Deleted: results-chart-without-patch.1.json 3.9 KB

results-chart-without-patch.1.json 3.9 KB View Download

Deleted: results-chart-with-patch.1.json 4.0 KB

results-chart-with-patch.1.json 4.0 KB View Download

color-changes.html doesn't have any text in it as far as I can see, skobes@ - would the autosizer have any effect on a textless page?

jaebaek@, some things to try: add some logging around your change and see if the codepath is even hit. Make sure you don't notice any differences between the with-patch and without-patch versions as the test runson the device. You could also count the number of times we enter layout or how many objects get layout - skobes@ might know the best way to do that.

That said, the metric recovered pretty quickly. The recovery CL range doesn't point out anythign that would obviously do that. Maybe it was just a hiccup with the bot? Unfortunately the ref build wasn't running at the time so we can't compare.

The autosizer shouldn't do anything if there's no text.

LocalFrameView::LayoutCount should tell you the number of layout passes the document has had.  You can also get more fine-grained by enabling the TrackLayoutPassesPerBlock REF and calling LayoutBlockFlow::GetLayoutPassCountForTesting.

bokan@, thank you for brilliant insights.
I checked the following changes:

C1.
https://chromium-review.googlesource.com/c/chromium/src/+/831814/11/third_party/WebKit/Source/core/layout/TextAutosizer.cpp#597

C2.
https://chromium-review.googlesource.com/c/chromium/src/+/831814/11/third_party/WebKit/Source/core/style/ComputedStyle.cpp

When I added logging, C2 was not executed and C1 was executed. However, C1 and the branch affected by C1 (i.e., line 614 of third_party/WebKit/Source/core/layout/TextAutosizer.cpp, the if branch based on page_info_.page_needs_autosizing_) are executed both with/without the CL in the same way.


skobes@,
As far as I understand, C1 is always executed regardless of text-autosizing, am I right?
And I am sorry but could you please let me know where I can get LocalFrameView::LayoutCount() to check the final number of layouts?

> As far as I understand, C1 is always executed regardless of text-autosizing, am I right?

TextAutosizer::UpdatePageInfo will be called regardless of whether text autosizing is enabled.

> And I am sorry but could you please let me know where I can get LocalFrameView::LayoutCount() to check the final number of layouts?

I'm not sure what you mean by final number.  I guess you could log the value of layout_count_ every time it is incremented.

Thank you, skobes@.

I printed out layout_count_ whenever it is increased (i.e., in LocalFrameView::UpdateLayout()). With/without the CL showed the same result as

01-17 11:49:13.864 31227 31242 E chromium: [ERROR:LocalFrameView.cpp(1343)] JAEBAEK UpdateLayout 1343 1
01-17 11:49:13.893 31227 31242 E chromium: [ERROR:LocalFrameView.cpp(1343)] JAEBAEK UpdateLayout 1343 2
01-17 11:49:13.894 31227 31242 E chromium: [ERROR:LocalFrameView.cpp(1343)] JAEBAEK UpdateLayout 1343 3
01-17 11:49:18.207 31227 31242 E chromium: [ERROR:LocalFrameView.cpp(1343)] JAEBAEK UpdateLayout 1343 1
01-17 11:50:17.204 31227 31242 E chromium: [ERROR:LocalFrameView.cpp(1343)] JAEBAEK UpdateLayout 1343 2
01-17 11:50:49.305 31227 31242 E chromium: [ERROR:LocalFrameView.cpp(1343)] JAEBAEK UpdateLayout 1343 3
01-17 11:50:49.305 31227 31242 E chromium: [ERROR:LocalFrameView.cpp(1343)] JAEBAEK UpdateLayout 1343 4


alexclarke@, based on various measurements including experiments in my local setup, code path check, and layout count check, it seems that the CL is not related to the performance degradation. In particular, the effective part of the CL (https://chromium-review.googlesource.com/c/chromium/src/+/831814/11/third_party/WebKit/Source/core/style/ComputedStyle.cpp) is not executed when running the performance test (color-changes.html). Could you please check it again and let me know if there is anything I must figure out?

I figure out what happened enough ..
It seems the performance degradation was from just a random fluctuation ..

The following are evidences:
1. The test pointed that my CL caused the performance degradation does not hit the code I changed.
2. I tried to reproduce it, but the test did not show the performance degradation.
3. One thing I found through my experiments is that the performance fluctuates a lot .. In particular, it changed depending on testing environments (e.g., the CPU frequency, priority of the task).

Let me close this bug.