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

commit 9e3f17630b4de8507b9edc3414784bc869f987b0
Author: David Bokan <bokan@chromium.org>
Date: Tue Aug 07 15:52:23 2018

[BlinkGenPropertyTrees] Update viewport only with flag

It seems this code may be responsible for performance regression in the
linked bug. This CL makes the code conditional on the blink gen property
trees flag (it's unneeded without the flag) to make sure its really the
cause of the regression. We can investigate performance improvements if
it's confirmed.

Bug:  868927 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I5a868f1f57429fe82cd01446b24d8466cc009e38
Reviewed-on: https://chromium-review.googlesource.com/1161205
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581237}
[modify] https://crrev.com/9e3f17630b4de8507b9edc3414784bc869f987b0/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc
[modify] https://crrev.com/9e3f17630b4de8507b9edc3414784bc869f987b0/third_party/blink/renderer/core/paint/paint_property_tree_update_tests.cc
[modify] https://crrev.com/9e3f17630b4de8507b9edc3414784bc869f987b0/third_party/blink/renderer/core/paint/pre_paint_tree_walk.cc

We've only got a single data point so far but it looks like the regression is reverting in the canary channel. I'll wait until Monday for some more data and we can request a merge.

Canary channel shows significant improvement so requesting merge.

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

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

Is the change listed at #2 fully safe to merge to M69?

Yes, we're just going back to an old, well-tested code path. The code that patch avoids isn't used until we turn on an off-by-default flag so this should be very low risk.

Approving merge to M69 branch 3497 based on comments #4 and #7. Please merge ASAP. Thank you.

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

commit abee2be4e1e29569572cb2baefb381d5756b5eef
Author: David Bokan <bokan@chromium.org>
Date: Tue Aug 14 20:59:29 2018

[BlinkGenPropertyTrees] Update viewport only with flag

It seems this code may be responsible for performance regression in the
linked bug. This CL makes the code conditional on the blink gen property
trees flag (it's unneeded without the flag) to make sure its really the
cause of the regression. We can investigate performance improvements if
it's confirmed.

Bug:  868927 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I5a868f1f57429fe82cd01446b24d8466cc009e38
Reviewed-on: https://chromium-review.googlesource.com/1161205
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#581237}(cherry picked from commit 9e3f17630b4de8507b9edc3414784bc869f987b0)
Reviewed-on: https://chromium-review.googlesource.com/1174974
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#632}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/abee2be4e1e29569572cb2baefb381d5756b5eef/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc
[modify] https://crrev.com/abee2be4e1e29569572cb2baefb381d5756b5eef/third_party/blink/renderer/core/paint/paint_property_tree_update_tests.cc
[modify] https://crrev.com/abee2be4e1e29569572cb2baefb381d5756b5eef/third_party/blink/renderer/core/paint/pre_paint_tree_walk.cc

David, can you investigate the performance of our approach with BlinkGenPropertyTrees? Is it possible to make this update cheaper?

Yup - will do. Pri 1->2 since it's no longer regressing stable users.

I still have to get through some of the other correctness work (things are still fairly busted with the flag on). Would you prefer I look at this first or fix Mac overscroll, RTL, etc.?

I think Xianzhu may get the RTL bug as part of fixing the layout test failures, so maybe you can ignore that for now. Mac overscroll or this SGTM!

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

commit 02246605274d8c4e7569e317b1dd95bf74caf263
Author: David Bokan <bokan@chromium.org>
Date: Wed Aug 22 19:40:57 2018

[blink-gen-property-trees] Optimize VisualViewport update

This patch does a few things to hopefully speed up the viewport update
during pre-paint:

- Only process overlay scrollbars if we're on a platform that uses
them.
- Cache the element id on the visual viewport rather than creating it
each time the getter is used.
- Use passed in node pointers for the current context, rather than
assuming we should use the root nodes.

With these changes, we also reenable running this viewport update in
!enable-blink-gen-property-trees mode.

Bug:  868927 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I161b8b1901cb2183f060130ecec058ca5f693170
Reviewed-on: https://chromium-review.googlesource.com/1183983
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585204}
[modify] https://crrev.com/02246605274d8c4e7569e317b1dd95bf74caf263/third_party/blink/renderer/core/frame/visual_viewport.cc
[modify] https://crrev.com/02246605274d8c4e7569e317b1dd95bf74caf263/third_party/blink/renderer/core/frame/visual_viewport.h
[modify] https://crrev.com/02246605274d8c4e7569e317b1dd95bf74caf263/third_party/blink/renderer/core/frame/visual_viewport_test.cc
[modify] https://crrev.com/02246605274d8c4e7569e317b1dd95bf74caf263/third_party/blink/renderer/core/layout/scrollbars_test.cc
[modify] https://crrev.com/02246605274d8c4e7569e317b1dd95bf74caf263/third_party/blink/renderer/core/paint/paint_property_tree_builder_test.cc
[modify] https://crrev.com/02246605274d8c4e7569e317b1dd95bf74caf263/third_party/blink/renderer/core/paint/paint_property_tree_update_tests.cc
[modify] https://crrev.com/02246605274d8c4e7569e317b1dd95bf74caf263/third_party/blink/renderer/core/paint/pre_paint_tree_walk.cc

I'll leave some of my findings for future reference:

The regression here is mostly or completely seen on 64 bit versions of Chrome (including on Android). My best guess is that 64 bit pointers push us past a cache line limit or something similar. The regression only happens in 50th and 25th percentiles and is only about 50 microseconds. High level profiling didn't show any obvious hot spots. I made a few improvements in the patch above that should improve a little. pdr@ suggested adding a dirty bit and avoiding the viewport update if nothing has changed. IMHO, the update is small enough that it's not worth the added complexity and we can reevaluate if we find PrePaint to be a bottleneck.

I think this is a great approach. The added complexity of dirty bits is not completely trivial.

Lets keep an eye on the 50%ile graph for a few days to make sure this doesn't re-regress:
https://uma.googleplex.com/p/chrome/timeline_v2?sid=d903c9b9449b9c1b02f4c4b399884bb5

Sadly, I think this resurfaced. The canary numbers have jumped back up.

Because visual viewport is self-contained, I don't think it will be too hard to add a needs_paint_property_update bit for it. For example, in VisualViewport::SetScrollOffset we'd add "needs_paint_property_update_ = true;" (similarly for all other fields read in VisualViewportPaintPropertyTreeBuilder::Update), then in VisualViewportPaintPropertyTreeBuilder::Update we'd just early-out if no update is needed.

Drats. Ok, I'll add that shortly.

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

commit b766e72e3ac981edae8755abc4f3c18bfa25d8d5
Author: David Bokan <bokan@chromium.org>
Date: Wed Aug 29 18:26:51 2018

[blink-gen-property-trees] Add dirty bit for viewport update

Skip updating visual viewport's paint property nodes unless one of the
inputs has changed.

Bug:  868927 ,877184
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;luci.chromium.try:linux-blink-gen-property-trees;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I62e37c449a14098175e79d6d357a5aecac314e9a
Reviewed-on: https://chromium-review.googlesource.com/1194564
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587204}
[modify] https://crrev.com/b766e72e3ac981edae8755abc4f3c18bfa25d8d5/third_party/blink/renderer/core/frame/visual_viewport.cc
[modify] https://crrev.com/b766e72e3ac981edae8755abc4f3c18bfa25d8d5/third_party/blink/renderer/core/frame/visual_viewport.h
[modify] https://crrev.com/b766e72e3ac981edae8755abc4f3c18bfa25d8d5/third_party/blink/renderer/core/fullscreen/fullscreen.cc
[modify] https://crrev.com/b766e72e3ac981edae8755abc4f3c18bfa25d8d5/third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc
[modify] https://crrev.com/b766e72e3ac981edae8755abc4f3c18bfa25d8d5/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc

Still keeping an eye on this - dev channel hasn't rolled the change yet.

Canary recovered but not all the way, perhaps enough that we're comfortable with it?

Any idea why canary didn't recover almost all the way? I could imagine we still are barely slower for the initial building of the viewport nodes, but after that, the dirty bit should make the original patch a no-op for perf. I wonder if we're accidentally setting the dirty bit in too many cases?

I'm okay not spending too much time on this (see discussion in https://chromium-review.googlesource.com/1194564).

I undid the change for M70 in  bug 879205 . See discussion there in #15 for reasons why this was causing a perf regression. Fixing for BGPT is tracked in  bug 888202