Viewport paint property nodes cause slow matrix transforms due to floating point error |
|||
Issue descriptionThis bug is split off from issue 879205 which tracked a paint perf regression in adding viewport nodes to the paint property tree. The regression was due to the fact that we now have scale involved during painting. In the benchmark, when painting a layer, the local transform has no scale applied, but because we had to undo the viewport scale the local transform matrix isn't quite an IdentityOrTranslationMatrix. This is due to some floating point error being introduced and some of the expected 1 and 0 values in the matrix are off by a tiny epsilon. The regression in 879205 was fixed by only adding the viewport nodes when BGPT is turned on. This bug is to track fixing the underlying issue so we can ship BGPT without the regression.
,
Oct 9
With https://crrev.com/596908 (Improve precision of GeometryMapper for 2d translations), we think the patch skipping these nodes (https://crrev.com/592538, Skip viewport update pre-BGPT) can be reverted.
,
Oct 9
I will try this today (I'm running into issues getting my Android build working so it's taking longer than expected...)
,
Oct 9
Ok - confirmed locally that reverting my patch no longer makes a difference on the metric. Thanks Xian Zhu! I've put up a revert
,
Oct 9
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
,
Oct 25
|
|||
►
Sign in to add a comment |
|||
Comment 1 by bokan@chromium.org
, Sep 22