Optimize PaintInvalidatorContext::mapLocalRectToVisualRectInBacking()

Optimize PaintInvalidatorContext::mapLocalRectToVisualRectInBacking()

Now pass GeometryMapper in PaintInvalidatorContext and use it in
mapLocalRectToVisualRectBacking().

The reason of the regression was that I removed the isEmpty() check
from ObjectPaintInvalidator::invalidateSelectionIfNeeded(), hoping
the function was fast if the rect is empty, but I didn't notice that
the function allocated GeometryMapper in each call. This caused cost
of one GeometryMapper allocation for each layout object invalidation.

Performance result on mac_10_12:
complex-content-slow-scroll
Before: 194.856 ms
With this CL: 147.111 ms

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

Review-Url: https://codereview.chromium.org/2763833003
Cr-Commit-Position: refs/heads/master@{#458888}
Committed: https://chromium.googlesource.com/chromium/src/+/b87cdfa7b1b594a30a76793904cf916c06048721

[Xianzhu, 2017-03-21 17:49:29]

Description was changed from

==========
Optimize PaintInvalidatorContext::mapLocalRectToVisualRectInBacking()

Now pass GeometryMapper in PaintInvalidatorContext and use it in
mapLocalRectToVisualRectBacking().

The reason of the regression was that I removed the isEmpty() check
from ObjectPaintInvalidator::invalidateSelectionIfNeeded(), hoping
the function was fast if the rect is empty, but I didn't notice that
the function allocated GeometryMapper in each call. This caused cost
of one GeometryMapper allocation for each layout object invalidation.

BUG=703111
==========

to

==========
Optimize PaintInvalidatorContext::mapLocalRectToVisualRectInBacking()

Now pass GeometryMapper in PaintInvalidatorContext and use it in
mapLocalRectToVisualRectBacking().

The reason of the regression was that I removed the isEmpty() check
from ObjectPaintInvalidator::invalidateSelectionIfNeeded(), hoping
the function was fast if the rect is empty, but I didn't notice that
the function allocated GeometryMapper in each call. This caused cost
of one GeometryMapper allocation for each layout object invalidation.

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

[Xianzhu, 2017-03-21 17:49:50]

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

[commit-bot: I haz the power, 2017-03-21 17:50:19]

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-03-21 20:26:56]

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

[commit-bot: I haz the power, 2017-03-21 20:26:57]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2017-03-22 16:59:27]

Description was changed from

==========
Optimize PaintInvalidatorContext::mapLocalRectToVisualRectInBacking()

Now pass GeometryMapper in PaintInvalidatorContext and use it in
mapLocalRectToVisualRectBacking().

The reason of the regression was that I removed the isEmpty() check
from ObjectPaintInvalidator::invalidateSelectionIfNeeded(), hoping
the function was fast if the rect is empty, but I didn't notice that
the function allocated GeometryMapper in each call. This caused cost
of one GeometryMapper allocation for each layout object invalidation.

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

to

==========
Optimize PaintInvalidatorContext::mapLocalRectToVisualRectInBacking()

Now pass GeometryMapper in PaintInvalidatorContext and use it in
mapLocalRectToVisualRectBacking().

The reason of the regression was that I removed the isEmpty() check
from ObjectPaintInvalidator::invalidateSelectionIfNeeded(), hoping
the function was fast if the rect is empty, but I didn't notice that
the function allocated GeometryMapper in each call. This caused cost
of one GeometryMapper allocation for each layout object invalidation.

Performance result on mac_10_12:
complex-content-slow-scroll
Before: 194.856 ms
With this CL: 147.111 ms

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

[Xianzhu, 2017-03-22 16:59:40]

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

[Xianzhu, 2017-03-22 17:00:10]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org

[chrishtr, 2017-03-22 17:54:31]

https://codereview.chromium.org/2763833003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/2763833003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:638: static
GeometryMapper& dummyGeometryMapper() {
Is it somehow guaranteed that this GeometryMapper is never used?

[wkorman, 2017-03-22 18:33:40]

lgtm

https://codereview.chromium.org/2763833003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/2763833003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:640:
(GeometryMapper::create()));
We saw pdr@ recently make a perf improvement by adding fast-malloc to an
instance, should we consider similar for GeometryMapper?
https://codereview.chromium.org/2744243010

Maybe it's not worth it just b/c we allocate comparatively few and not in hot
code path.

[Xianzhu, 2017-03-22 18:50:02]

https://codereview.chromium.org/2763833003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/2763833003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:638: static
GeometryMapper& dummyGeometryMapper() {
On 2017/03/22 17:54:31, chrishtr wrote:
> Is it somehow guaranteed that this GeometryMapper is never used?

Yes because the adapter's mapLocalRectToVisualRectInBacking() overrides the
method in PaintInvalidatorContext's. The overriding method uses
PaintInvaldiationState to map the rect.

Create a followup patch to enforce it.

[Xianzhu, 2017-03-22 18:50:48]

https://codereview.chromium.org/2763833003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/2763833003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:638: static
GeometryMapper& dummyGeometryMapper() {
On 2017/03/22 18:50:02, Xianzhu wrote:
> On 2017/03/22 17:54:31, chrishtr wrote:
> > Is it somehow guaranteed that this GeometryMapper is never used?
> 
> Yes because the adapter's mapLocalRectToVisualRectInBacking() overrides the
> method in PaintInvalidatorContext's. The overriding method uses
> PaintInvaldiationState to map the rect.
> 
> Create a followup patch to enforce it:
https://codereview.chromium.org/2772473003

[Xianzhu, 2017-03-22 18:51:38]

https://codereview.chromium.org/2763833003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/2763833003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:640:
(GeometryMapper::create()));
On 2017/03/22 18:33:40, wkorman wrote:
> We saw pdr@ recently make a perf improvement by adding fast-malloc to an
> instance, should we consider similar for GeometryMapper?
> https://codereview.chromium.org/2744243010
> 
> Maybe it's not worth it just b/c we allocate comparatively few and not in hot
> code path.

I think it's not worth it.

[chrishtr, 2017-03-22 18:56:22]

lgtm

https://codereview.chromium.org/2763833003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/2763833003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:638: static
GeometryMapper& dummyGeometryMapper() {
On 2017/03/22 at 18:50:48, Xianzhu wrote:
> On 2017/03/22 18:50:02, Xianzhu wrote:
> > On 2017/03/22 17:54:31, chrishtr wrote:
> > > Is it somehow guaranteed that this GeometryMapper is never used?
> > 
> > Yes because the adapter's mapLocalRectToVisualRectInBacking() overrides the
> > method in PaintInvalidatorContext's. The overriding method uses
> > PaintInvaldiationState to map the rect.
> > 
> > Create a followup patch to enforce it:
https://codereview.chromium.org/2772473003

Ok, please add a comment saying that it's guaranteed by code structure that this
dummy object
is never called.

[Xianzhu, 2017-03-22 19:20:43]

https://codereview.chromium.org/2763833003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp (right):

https://codereview.chromium.org/2763833003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp:638: static
GeometryMapper& dummyGeometryMapper() {
On 2017/03/22 18:56:21, chrishtr wrote:
> On 2017/03/22 at 18:50:48, Xianzhu wrote:
> > On 2017/03/22 18:50:02, Xianzhu wrote:
> > > On 2017/03/22 17:54:31, chrishtr wrote:
> > > > Is it somehow guaranteed that this GeometryMapper is never used?
> > > 
> > > Yes because the adapter's mapLocalRectToVisualRectInBacking() overrides
the
> > > method in PaintInvalidatorContext's. The overriding method uses
> > > PaintInvaldiationState to map the rect.
> > > 
> > > Create a followup patch to enforce it:
> https://codereview.chromium.org/2772473003
> 
> Ok, please add a comment saying that it's guaranteed by code structure that
this
> dummy object
> is never called.

Done.

[Xianzhu, 2017-03-22 19:20:49]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-03-22 19:20:49]

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

[commit-bot: I haz the power, 2017-03-22 19:21:20]

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-03-22 22:04:08]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490210449412860,
"parent_rev": "a6651f1e3ed07e5c6a394664723bb6833c01a387", "commit_rev":
"b87cdfa7b1b594a30a76793904cf916c06048721"}

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

Description was changed from

==========
Optimize PaintInvalidatorContext::mapLocalRectToVisualRectInBacking()

Now pass GeometryMapper in PaintInvalidatorContext and use it in
mapLocalRectToVisualRectBacking().

The reason of the regression was that I removed the isEmpty() check
from ObjectPaintInvalidator::invalidateSelectionIfNeeded(), hoping
the function was fast if the rect is empty, but I didn't notice that
the function allocated GeometryMapper in each call. This caused cost
of one GeometryMapper allocation for each layout object invalidation.

Performance result on mac_10_12:
complex-content-slow-scroll
Before: 194.856 ms
With this CL: 147.111 ms

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

to

==========
Optimize PaintInvalidatorContext::mapLocalRectToVisualRectInBacking()

Now pass GeometryMapper in PaintInvalidatorContext and use it in
mapLocalRectToVisualRectBacking().

The reason of the regression was that I removed the isEmpty() check
from ObjectPaintInvalidator::invalidateSelectionIfNeeded(), hoping
the function was fast if the rect is empty, but I didn't notice that
the function allocated GeometryMapper in each call. This caused cost
of one GeometryMapper allocation for each layout object invalidation.

Performance result on mac_10_12:
complex-content-slow-scroll
Before: 194.856 ms
With this CL: 147.111 ms

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

Review-Url: https://codereview.chromium.org/2763833003
Cr-Commit-Position: refs/heads/master@{#458888}
Committed:
https://chromium.googlesource.com/chromium/src/+/b87cdfa7b1b594a30a76793904cf...
==========

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

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/b87cdfa7b1b594a30a76793904cf...