Reduce the requirement for saving previousLocation

Reduce the requirement for saving previousLocation

ObjectPaintInvalidator::previousLocationInBacking() accesses a global
map if location was different from visualRect.location() during the last
paint invalidation.

The percentage of LayoutObjects requiring to save previousLocation was
about 5% before SlimmingPaintInvalidation is enabled, but increased to
about 37% after.

The main reason was that we always called enclosingIntRect() when
computing visual rect, causing visualRect.location() was different from
location if location is not at whole pixels. Now call enclosingIntRect()
only when there is any transform.

A minor reason was about empty rects having different location. Location
of an empty visual rect actually has no actual functional effect on
paint invalidation. Now don't map empty rects, but force its location
to be the same as location in backing, so that we always don't need to
save previous location of empty visual rects.

BUG=685179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2706063003
Cr-Commit-Position: refs/heads/master@{#451908}
Committed: https://chromium.googlesource.com/chromium/src/+/8cc1739f3017dca5bc97d2721bcdfc903eaec6ec

[Xianzhu, 2017-02-21 02:06:13]

Description was changed from

==========
Reduce the requirement for saving previousLocation

ObjectPaintInvalidator::previousLocationInBacking() accesses a global
map if location was different from visualRect.location() during the last
paint invalidation.

The percentage of LayoutObjects requiring to save previousLocation was
about 5% before SlimmingPaintInvalidation is enabled, but increased to
about 37% after.

The main reason was that we always called enclosingIntRect() when
computing visual rect, causing visualRect.location() was different from
location if location is not at whole pixels. Now call enclosingIntRect()
only when there is any transform.

A minor reason was about empty rects having different location. Location
of an empty visual rect actually has no actual functional effect on
paint invalidation. Now don't map empty rects, but force its location
to be the same as location in backing, so that we always don't need to
save previous location of empty visual rects.

BUG=685179
==========

to

==========
Reduce the requirement for saving previousLocation

ObjectPaintInvalidator::previousLocationInBacking() accesses a global
map if location was different from visualRect.location() during the last
paint invalidation.

The percentage of LayoutObjects requiring to save previousLocation was
about 5% before SlimmingPaintInvalidation is enabled, but increased to
about 37% after.

The main reason was that we always called enclosingIntRect() when
computing visual rect, causing visualRect.location() was different from
location if location is not at whole pixels. Now call enclosingIntRect()
only when there is any transform.

A minor reason was about empty rects having different location. Location
of an empty visual rect actually has no actual functional effect on
paint invalidation. Now don't map empty rects, but force its location
to be the same as location in backing, so that we always don't need to
save previous location of empty visual rects.

BUG=685179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2017-02-21 02:07:46]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 02:08:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 03:22:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 03:22:48]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2017-02-21 05:05:23]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 05:06:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2017-02-21 05:06:05]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 05:06:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 06:39:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 06:39:56]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Xianzhu, 2017-02-21 07:01:40]

The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 07:02:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 09:55:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 09:55:07]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Xianzhu, 2017-02-21 17:10:23]

wangxianzhu@chromium.org changed reviewers:
+ pdr@chromium.org, wkorman@chromium.org

[Xianzhu, 2017-02-21 17:10:23]

https://codereview.chromium.org/2706063003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/flexbox/repaint-rtl-column-expected.txt
(right):

https://codereview.chromium.org/2706063003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/flexbox/repaint-rtl-column-expected.txt:22:
"reason": "incremental"
This expectation is copied from virtual/disable-spinvalidation.

This change makes these tests, which were previously forced full paint
invalidation because location and visualRect.location() are different, now
incremental invalidation applicable. This actually makes the
SlimmingPaintInvalidation path and the non-SlimmingPaintInvalidation path
generate the same results.

[wkorman, 2017-02-21 22:54:19]

lgtm

https://codereview.chromium.org/2706063003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxPaintInvalidatorTest.cpp (right):

https://codereview.chromium.org/2706063003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPaintInvalidatorTest.cpp:147:
TEST_P(BoxPaintInvalidatorTest, SubpixelVisualRectChagneWithTransform) {
Chagne -> Change

https://codereview.chromium.org/2706063003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2706063003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:118: // snapping, when
transforms are applied.
Are transforms the only potential cause of subpixel variance?

https://codereview.chromium.org/2706063003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:359: // visual rect
change for layout caused invalidation.
insert 'whether a' before 'visual'

https://codereview.chromium.org/2706063003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:363:
context.newLocation.move(adjustment);
We don't need this for the text case?

[Xianzhu, 2017-02-21 23:15:59]

https://codereview.chromium.org/2706063003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxPaintInvalidatorTest.cpp (right):

https://codereview.chromium.org/2706063003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPaintInvalidatorTest.cpp:147:
TEST_P(BoxPaintInvalidatorTest, SubpixelVisualRectChagneWithTransform) {
On 2017/02/21 22:54:19, wkorman wrote:
> Chagne -> Change

Done.

https://codereview.chromium.org/2706063003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInvalidator.cpp (right):

https://codereview.chromium.org/2706063003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:118: // snapping, when
transforms are applied.
On 2017/02/21 22:54:19, wkorman wrote:
> Are transforms the only potential cause of subpixel variance?

Yes.

We need to apply enclosingIntRect before earlier because 
 transform(enclosingIntRect(rect))) may differ from
enclosingIntRect(transform(rect)).

In other cases, we can always keep visualRect subpixel-aware and apply
enclosingIntRect() in the last step (in
CompositedLayerMapping::setContentsNeedDisplayInRect()).

Updated comment to mention the other cases.

https://codereview.chromium.org/2706063003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:359: // visual rect
change for layout caused invalidation.
On 2017/02/21 22:54:19, wkorman wrote:
> insert 'whether a' before 'visual'

Done.

https://codereview.chromium.org/2706063003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidator.cpp:363:
context.newLocation.move(adjustment);
On 2017/02/21 22:54:19, wkorman wrote:
> We don't need this for the text case?

No, because context.newVisualRect.location() has already adjustment applied.

[Xianzhu, 2017-02-21 23:16:07]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-02-21 23:16:08]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2706063003/#ps60001
(title: "-")

[commit-bot: I haz the power, 2017-02-21 23:18:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 01:21:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 01:22:01]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Xianzhu, 2017-02-22 01:52:24]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-02-22 01:53:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 03:56:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 03:56:13]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Xianzhu, 2017-02-22 05:02:45]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-02-22 05:03:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 07:11:40]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487739765495840,
"parent_rev": "b3d8f58c9c4070637f727fa46eec5c9a367c8dcd", "commit_rev":
"8cc1739f3017dca5bc97d2721bcdfc903eaec6ec"}

[commit-bot: I haz the power, 2017-02-22 07:12:38]

Description was changed from

==========
Reduce the requirement for saving previousLocation

ObjectPaintInvalidator::previousLocationInBacking() accesses a global
map if location was different from visualRect.location() during the last
paint invalidation.

The percentage of LayoutObjects requiring to save previousLocation was
about 5% before SlimmingPaintInvalidation is enabled, but increased to
about 37% after.

The main reason was that we always called enclosingIntRect() when
computing visual rect, causing visualRect.location() was different from
location if location is not at whole pixels. Now call enclosingIntRect()
only when there is any transform.

A minor reason was about empty rects having different location. Location
of an empty visual rect actually has no actual functional effect on
paint invalidation. Now don't map empty rects, but force its location
to be the same as location in backing, so that we always don't need to
save previous location of empty visual rects.

BUG=685179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Reduce the requirement for saving previousLocation

ObjectPaintInvalidator::previousLocationInBacking() accesses a global
map if location was different from visualRect.location() during the last
paint invalidation.

The percentage of LayoutObjects requiring to save previousLocation was
about 5% before SlimmingPaintInvalidation is enabled, but increased to
about 37% after.

The main reason was that we always called enclosingIntRect() when
computing visual rect, causing visualRect.location() was different from
location if location is not at whole pixels. Now call enclosingIntRect()
only when there is any transform.

A minor reason was about empty rects having different location. Location
of an empty visual rect actually has no actual functional effect on
paint invalidation. Now don't map empty rects, but force its location
to be the same as location in backing, so that we always don't need to
save previous location of empty visual rects.

BUG=685179
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2706063003
Cr-Commit-Position: refs/heads/master@{#451908}
Committed:
https://chromium.googlesource.com/chromium/src/+/8cc1739f3017dca5bc97d2721bcd...
==========

[commit-bot: I haz the power, 2017-02-22 07:12:39]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8cc1739f3017dca5bc97d2721bcd...