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

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


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

Android Nexus5 Perf

rasterize_and_record_micro.top_25 - Benchmark documentation link:
  None

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/1323c62b640000

😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/1323c62b640000

The swarming task expired. The bots are likely overloaded, dead, or misconfigured.

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/17c4dfdb640000

📍 Found significant differences after each of 15 commits.
https://pinpoint-dot-chromeperf.appspot.com/job/17c4dfdb640000

inspector: do not convert and store String16 for script source by kozyatinskiy@chromium.org
https://chromium.googlesource.com/v8/v8/+/e987606a8a187d62e24bd16d7fbda9e786d6a247
1.029 → No values

[scopes] Clean up and centralize mapped/unmapped arguments logic by adamk@chromium.org
https://chromium.googlesource.com/v8/v8/+/91041c1260788b823895996b8a522d9e40b58c13
No values → 1.049

Revert "[builtins] Reland Array.prototype.splice() Torque implementation." by tebbi@chromium.org
https://chromium.googlesource.com/v8/v8/+/e99a109281ff0e73cae94c6b23db0cb96407de22
1.049 → 1.039 (-0.0102)

Revert "inspector: do not convert and store String16 for script source" by mslekova@chromium.org
https://chromium.googlesource.com/v8/v8/+/6b860b6977186dea0e81c7f57fda615f8a7be61d
1.039 → 1.03 (-0.0089)

[wasm] Ensure all wasm runtime stubs are PIC by jgruber@chromium.org
https://chromium.googlesource.com/v8/v8/+/e5e30b34637ce2cbeb505ab989d59a05d19943f5
1.024 → No values

[turbofan] Serialize more data. by neis@chromium.org
https://chromium.googlesource.com/v8/v8/+/d67f0a05d90704403ccd17115469152aafe3c59f
No values → 1.041

[cleanup] Use ZoneChunkList in SafepointTableBuiler by petermarshall@chromium.org
https://chromium.googlesource.com/v8/v8/+/3f1e2346b4db7fdea1518441a6ed7e5ffa6b13d9
1.041 → 1.05 (+0.0086)

Revert "[scanner] Add Skip to be used after successful Peek" by mslekova@chromium.org
https://chromium.googlesource.com/v8/v8/+/9fa501597954eecc92bbbaf973b3b700f1073427
1.048 → No values

Revert "Stop logging Builtin functions as LazyCompile." by sigurds@chromium.org
https://chromium.googlesource.com/v8/v8/+/a3e1decd7bc2848524c15703097c451add4bfaec
No values → 1.051

Fix JS dialog dismissal cause - reland by yaoxia@chromium.org
https://chromium.googlesource.com/chromium/src/+/ee6dac2e080228132192bdd4f95cff3921375c65
1.053 → 1.042 (-0.01108)

[signin] Remove code from Dice Milestone 2 by droger@chromium.org
https://chromium.googlesource.com/chromium/src/+/a67998f93a1bc8a1be0faa08fa62b93fbbd417f3
1.04 → 1.035 (-0.0054)

Portals: Create the portal WebContents. by lfg@chromium.org
https://chromium.googlesource.com/chromium/src/+/98a5bafd8acc36237dab02b86a03f681ad484df8
1.035 → 1.029 (-0.0059)

Revert "[Image Capture, Android] Fix various threading issues" by chfremer@chromium.org
https://chromium.googlesource.com/chromium/src/+/c8198744b21d85984478c10a765e808ca9b49610
1.029 → 1.038 (+0.0087)

service worker: Use a callback instead of an observer for pause after download. by falken@chromium.org
https://chromium.googlesource.com/chromium/src/+/fd6ffa7060dcc92f59736d194141ab6571a5d1c3
1.043 → 1.028 (-0.015)

[blink-gen-property-trees] Optimize VisualViewport update by bokan@chromium.org
https://chromium.googlesource.com/chromium/src/+/02246605274d8c4e7569e317b1dd95bf74caf263
1.031 → 1.132 (+0.1013)

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Benchmark documentation link:
  None

-cc everyone, other differences look like just noise.

+RBS since this made it into M70 branch.

This issue is marked as a release blocker with no OS labels associated. Please add an appropriate OS label.

All release blocking issues should have OS labels associated to it, so that the issue can tracked and promptly verified, once it gets fixed.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This bug seems to only occur on the Nexus 5 and didn't get better when we gated the viewport update behind a dirty bit. Maybe this'll help explain why  bug 868927  didn't improve all the way back to baseline?

I've got my hands on one so I'll see if I can repro locally. 

Philip, this regression (and I'm guessing the related one in  bug 868927 ) isn't directly caused by running VisualViewport::UpdatePaintPropertyNodesIfNeeded, it's caused by having extra nodes in the paint property tree. (Resetting the context back to root directly after UpdatePaintPropertyNodes fixes the regression).

I also disabled the viewport layer states and update in PAC and it makes no difference. IIRC, the paint property tree pre-BGPT is used just for invalidation, is that right? One additional clue, on the perf bots, this only occurs only on the Nexus 5, not the 5X or 6 nor any desktop platforms.

I need to poke at this some more but my N5 is on KitKat which doesn't support our profiling setup - I'll see if I can flash it up to N. In the mean time, any tips on where to look?

Nice investigation!

It's curious that a few extra nodes would cause such a regression. Can you take a cpu profile with and without the nodes and compare?

Is this regression important? Can you look at a trace and see which trace events are changing? This may be a microbenchmark of property trees, or it may be a real issue.

Yeah, it's a large page (desktop google SRP) with lots going on so it's shocking that adding ~3 nodes results in a 10% drop. I'll try getting some profiles today.

I'm going to land a patch in ToT to skip the viewport update in !BGPT mode (as we did before) so that I can merge to M70 and avoid regressing stable while I keep investigating the root cause here.

^^^ Forgot I need to update tests to do that so the CL wasn't as simple as I thought, will do that tomorrow.

I do think I've finally gotten to the bottom of this though:

Turns out that running the viewport update in PrePaint causes ChunkToLayerMapper::MapVisualRect to take longer (as called from GraphicsLayer::PaintContentsToDisplayList -> PaintChunksToCcLayer::ConvertInto). The inputs and outputs are identical as far as I could tell so this was curious.

Adding the viewport nodes causes us to have an ever so slightly different transform_ in ChunkToLayerMapper. Printing it out shows it was just a layer offset but digging deeper showed that IsIdentityOrTranslation returns false! Making the printing more verbose shows the transform matrix has tiny epsilon sized differences from 0/1 in the matrix to make it seem like a more general transform than it is. That causes us to miss the fast-path in TransformationMatrix::MapRect.

It's not so much adding the viewport nodes that causes the regression. Clearing the scale to 1 in VisualViewport::UpdatePaintPropertyNodesIfNeeded removed the regression as well. The benchmark here is a desktop page that loads zoomed out so this explains where the non-translate part of the transform comes from. As we walk into layer space, we lose a bit of precision undoing the scale.

As for why this repros only on the N5 - I'm not certain. It might just be an unlucky screen size and page scale factor. Running the same benchmark on my Nexus 6 shows that the matrix is indeed IdentityOrTranslation. Rounding to 1/0 if off by a tiny epsilon fixes the regression on the Nexus 5 as well. 

pdr@: I'm not sure what the right fix is here. I'll skip the update step if BGPT is off which will prevent regressing M70 but I think this will still be an issue when we turn on BGPT. Rounding is probably the wrong solution. We could have a special case when we walk out of the visual viewport since a non-0 scale is not going to be rare but I'm not sure how much of a problem this is outside of this micro-benchmark. WDYT?

Do you think GeometryMapper could have a bug where some math is being done wrong (or could be re-arranged to have less floating point error)? Chaopeng and I hit something very similar in https://chromium-review.googlesource.com/c/chromium/src/+/1210384 and we had to add TransformationMatrix::ApproximatelyEquals to make a DCHECK pass. TransformationMatrix is backed by doubles but most blink code uses floats--I wonder if we could be picking up an epsilon converting between doubles and floats? Can you try to poke around in the mapping code? This would be my preferred fix because the same bug likely affects other parts of our code too.

An option is to move Chaopeng's TransformationMatrix::ApproximatelyEquals out of DCHECK_IS_ON and use it to implement something like TransformationMatrix::IsApproximatelyIdentityOrTranslation.

+1 to the expedient M70 fix of putting this behind BlinkGenPropertyTrees.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d0b9a4ba2465b81ebf85031c53d94c156ce795c5

commit d0b9a4ba2465b81ebf85031c53d94c156ce795c5
Author: David Bokan <bokan@chromium.org>
Date: Wed Sep 19 20:40:52 2018

[blink-gen-property-trees] Skip viewport update pre-BGPT

Adding viewport nodes causes a performance regression so this CL turns
off the paint tree update in VisualViewport if BGPT (and SPv2) are off
since the nodes aren't needed.

Bug:  879205 
Cq-Include-Trybots: luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I19060e1d487d50ac9b65ae7bdaa32dba2e5caaa7
Reviewed-on: https://chromium-review.googlesource.com/1228489
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592538}
[modify] https://crrev.com/d0b9a4ba2465b81ebf85031c53d94c156ce795c5/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc
[modify] https://crrev.com/d0b9a4ba2465b81ebf85031c53d94c156ce795c5/third_party/blink/renderer/core/paint/paint_property_tree_update_tests.cc
[modify] https://crrev.com/d0b9a4ba2465b81ebf85031c53d94c156ce795c5/third_party/blink/renderer/core/paint/pre_paint_tree_walk.cc

Graph recovered - requesting merge to M70

This bug requires manual review: M70 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Approved for merge to 70.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d655378abac2af6a8c04520750f62352fcd1fa58

commit d655378abac2af6a8c04520750f62352fcd1fa58
Author: David Bokan <bokan@chromium.org>
Date: Sat Sep 22 00:08:53 2018

Merge: [blink-gen-property-trees] Skip viewport update pre-BGPT

Adding viewport nodes causes a performance regression so this CL turns
off the paint tree update in VisualViewport if BGPT (and SPv2) are off
since the nodes aren't needed.

TBR=pdr@chromium.org
(cherry picked from commit d0b9a4ba2465b81ebf85031c53d94c156ce795c5)

Bug:  879205 
Cq-Include-Trybots: luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I19060e1d487d50ac9b65ae7bdaa32dba2e5caaa7
Reviewed-on: https://chromium-review.googlesource.com/1228489
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#592538}
Reviewed-on: https://chromium-review.googlesource.com/1239693
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#578}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/d655378abac2af6a8c04520750f62352fcd1fa58/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc
[modify] https://crrev.com/d655378abac2af6a8c04520750f62352fcd1fa58/third_party/blink/renderer/core/paint/paint_property_tree_update_tests.cc
[modify] https://crrev.com/d655378abac2af6a8c04520750f62352fcd1fa58/third_party/blink/renderer/core/paint/pre_paint_tree_walk.cc

The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/d655378abac2af6a8c04520750f62352fcd1fa58

Commit: d655378abac2af6a8c04520750f62352fcd1fa58
Author: bokan@chromium.org
Commiter: bokan@chromium.org
Date: 2018-09-22 00:08:53 +0000 UTC

Merge: [blink-gen-property-trees] Skip viewport update pre-BGPT

Adding viewport nodes causes a performance regression so this CL turns
off the paint tree update in VisualViewport if BGPT (and SPv2) are off
since the nodes aren't needed.

TBR=pdr@chromium.org
(cherry picked from commit d0b9a4ba2465b81ebf85031c53d94c156ce795c5)

Bug:  879205 
Cq-Include-Trybots: luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I19060e1d487d50ac9b65ae7bdaa32dba2e5caaa7
Reviewed-on: https://chromium-review.googlesource.com/1228489
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#592538}
Reviewed-on: https://chromium-review.googlesource.com/1239693
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#578}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}

The fix is merged to M70. I'll track fixing the underlying issue in a new  bug 888202 .

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1aad28fdfd83d1241de2ee0dd24ca10659d9c9d6

commit 1aad28fdfd83d1241de2ee0dd24ca10659d9c9d6
Author: David Bokan <bokan@chromium.org>
Date: Tue Oct 09 22:00:32 2018

Revert "[blink-gen-property-trees] Skip viewport update pre-BGPT"

This reverts commit d0b9a4ba2465b81ebf85031c53d94c156ce795c5.

Reason for revert: The perf issue was fixed by https://crrev.com/596908

Original change's description:
> [blink-gen-property-trees] Skip viewport update pre-BGPT
>
> Adding viewport nodes causes a performance regression so this CL turns
> off the paint tree update in VisualViewport if BGPT (and SPv2) are off
> since the nodes aren't needed.
>
> Bug:  879205 
> Cq-Include-Trybots: luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
> Change-Id: I19060e1d487d50ac9b65ae7bdaa32dba2e5caaa7
> Reviewed-on: https://chromium-review.googlesource.com/1228489
> Reviewed-by: Philip Rogers <pdr@chromium.org>
> Commit-Queue: David Bokan <bokan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#592538}

TBR=bokan@chromium.org,pdr@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  879205 , 888202 
Change-Id: Ia47c3eb1e2c2a1624df9cf2f117eac9cfd7efc94
Cq-Include-Trybots: luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://chromium-review.googlesource.com/c/1272017
Reviewed-by: Philip Rogers <pdr@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598099}
[modify] https://crrev.com/1aad28fdfd83d1241de2ee0dd24ca10659d9c9d6/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc
[modify] https://crrev.com/1aad28fdfd83d1241de2ee0dd24ca10659d9c9d6/third_party/blink/renderer/core/paint/paint_property_tree_update_tests.cc
[modify] https://crrev.com/1aad28fdfd83d1241de2ee0dd24ca10659d9c9d6/third_party/blink/renderer/core/paint/pre_paint_tree_walk.cc