Reduce copying of local data structures in GeometryMapper and PaintLayerClipper.

Reduce copying of local data structures in GeometryMapper and PaintLayerClipper.

In particular, changes return values from FloatClipRect to const FloatClipRect&,
to avoid extra data structure copies.

In local testing, this CL produces up to a 10% improvement on the
rasterize_and_record_micro.partial_invalidation benchmark.

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

Review-Url: https://codereview.chromium.org/2745563004
Cr-Commit-Position: refs/heads/master@{#456011}
Committed: https://chromium.googlesource.com/chromium/src/+/05c00d2035047fa9d210d95afedf8bd063bccc33

[chrishtr, 2017-03-09 19:59:27]

Description was changed from

==========
none


none

BUG=
==========

to

==========
none


none

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

[chrishtr, 2017-03-09 20:21:40]

Description was changed from

==========
none


none

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

to

==========
Reduce copying of local data structures in GeometryMapper and PaintLayerClipper.

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

[chrishtr, 2017-03-09 22:33:31]

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

[commit-bot: I haz the power, 2017-03-09 22:34:43]

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

[chrishtr, 2017-03-09 22:38:09]

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

[commit-bot: I haz the power, 2017-03-09 22:38:42]

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

[chrishtr, 2017-03-09 22:39:06]

Description was changed from

==========
Reduce copying of local data structures in GeometryMapper and PaintLayerClipper.

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

to

==========
Reduce copying of local data structures in GeometryMapper and PaintLayerClipper.

In local testing, this CL produces up to a 10% improvement on the
rasterize_and_record_micro.partial_invalidation benchmark.

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

[chrishtr, 2017-03-09 23:25:36]

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

[chrishtr, 2017-03-09 23:37:26]

Description was changed from

==========
Reduce copying of local data structures in GeometryMapper and PaintLayerClipper.

In local testing, this CL produces up to a 10% improvement on the
rasterize_and_record_micro.partial_invalidation benchmark.

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

to

==========
Reduce copying of local data structures in GeometryMapper and PaintLayerClipper.

In particular, changes return values from FloatClipRect to const FloatClipRect&,
to avoid extra data structure copies.

In local testing, this CL produces up to a 10% improvement on the
rasterize_and_record_micro.partial_invalidation benchmark.

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

[Xianzhu, 2017-03-10 00:06:49]

https://codereview.chromium.org/2745563004/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):

https://codereview.chromium.org/2745563004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:141: if
(!object.hasLayer())
(Can be follow-up) We can tighten this condition by checking some fast subset of
conditions in shouldCreateSubsequence() in PaintLayerPainter.cpp, to increase
chance of early returns.

https://codereview.chromium.org/2745563004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:149: 
(Can be follow-up) We can also check for some fast subset of conditions in
shouldCreateSubsequence() in PaintLayerPainter.cpp to early return if we won't
create subsequnece for the layer.

https://codereview.chromium.org/2745563004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:154:
*ancestorPaintProperties.localBorderBoxProperties();
(Can be follow-up) If using the condition I proposed below line 178, the above
statement can be moved into the condition block. We can use
   const auto* ancestorClip =
ancestorPaintProperties.localBorderBoxProperties()->clip()
instead of ancestorState before the condition.

https://codereview.chromium.org/2745563004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:178: RefPtr<ClipRects>
clipRects = ClipRects::create();
(Can be follow-up) We can also skip line 178-210 if there won't be any clip,
like:
  RefPtr<ClipRects> clipRects;
  if (ancestorClip != context.treeBuilderContext.current.clip &&
      ancestorClip != context.treeBuilderContext.fixedPosition.clip &&
      ancestorClip != context.treeBuilderContext.absolutePositon.clip) {
    ...
  }
  ClipRects* previousClipRects = ...
  if ((!clipRects && !previousClipRects) ||
      (clipRects && previousClipRects && *clipRects == *previousClipRects))
    return;

  clear flags;
  ...

https://codereview.chromium.org/2745563004/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp
(left):

https://codereview.chromium.org/2745563004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp:33: 
Nit: Keep the blank line.

https://codereview.chromium.org/2745563004/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp
(right):

https://codereview.chromium.org/2745563004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp:90: const
FloatClipRect& GeometryMapper::localToAncestorVisualRectInternal(
It seems that we always return a temporary rect from this function, so returning
a reference seems not to worth the complexity.

Can we just use an output parameter?

[commit-bot: I haz the power, 2017-03-10 00:15:43]

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

[commit-bot: I haz the power, 2017-03-10 00:15:44]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2017-03-10 01:45:30]

https://codereview.chromium.org/2745563004/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right):

https://codereview.chromium.org/2745563004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:178: RefPtr<ClipRects>
clipRects = ClipRects::create();
On 2017/03/10 at 00:06:49, Xianzhu wrote:
> (Can be follow-up) We can also skip line 178-210 if there won't be any clip,
like:
>   RefPtr<ClipRects> clipRects;
>   if (ancestorClip != context.treeBuilderContext.current.clip &&
>       ancestorClip != context.treeBuilderContext.fixedPosition.clip &&
>       ancestorClip != context.treeBuilderContext.absolutePositon.clip) {
>     ...
>   }
>   ClipRects* previousClipRects = ...
>   if ((!clipRects && !previousClipRects) ||
>       (clipRects && previousClipRects && *clipRects == *previousClipRects))
>     return;
> 
>   clear flags;
>   ...

Do you mean if ancestorState.clip() is above all of those? Yes. I wonder if this
would
improve things though. It would avoid some allocations though. Will try in a
followup.

https://codereview.chromium.org/2745563004/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp
(left):

https://codereview.chromium.org/2745563004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp:33: 
On 2017/03/10 at 00:06:49, Xianzhu wrote:
> Nit: Keep the blank line.

Done.

https://codereview.chromium.org/2745563004/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp
(right):

https://codereview.chromium.org/2745563004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp:90: const
FloatClipRect& GeometryMapper::localToAncestorVisualRectInternal(
On 2017/03/10 at 00:06:49, Xianzhu wrote:
> It seems that we always return a temporary rect from this function, so
returning a reference seems not to worth the complexity.
> 
> Can we just use an output parameter?

Done. Good idea, it made the code a lot simpler, letting me remove a bunch of
hasRadius stuff since
visual rect use cases don't care about rounded corners.

[chrishtr, 2017-03-10 01:46:23]

https://codereview.chromium.org/2745563004/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapperTest.cpp
(right):

https://codereview.chromium.org/2745563004/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapperTest.cpp:425:
clipRect2.setRect(clip2->clipRect().rect());
Somehow I fixed a bug in this CL - previously it was incorrectly setting the
hasRadius bit.

[Xianzhu, 2017-03-10 04:01:16]

lgtm

[chrishtr, 2017-03-10 05:05:36]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2017-03-10 05:05:50]

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-10 06:50:36]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1489122336732400,
"parent_rev": "04b3fc7475401166b44c14b201fb957a66436f1d", "commit_rev":
"05c00d2035047fa9d210d95afedf8bd063bccc33"}

[commit-bot: I haz the power, 2017-03-10 06:51:30]

Description was changed from

==========
Reduce copying of local data structures in GeometryMapper and PaintLayerClipper.

In particular, changes return values from FloatClipRect to const FloatClipRect&,
to avoid extra data structure copies.

In local testing, this CL produces up to a 10% improvement on the
rasterize_and_record_micro.partial_invalidation benchmark.

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

to

==========
Reduce copying of local data structures in GeometryMapper and PaintLayerClipper.

In particular, changes return values from FloatClipRect to const FloatClipRect&,
to avoid extra data structure copies.

In local testing, this CL produces up to a 10% improvement on the
rasterize_and_record_micro.partial_invalidation benchmark.

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

Review-Url: https://codereview.chromium.org/2745563004
Cr-Commit-Position: refs/heads/master@{#456011}
Committed:
https://chromium.googlesource.com/chromium/src/+/05c00d2035047fa9d210d95afedf...
==========

[commit-bot: I haz the power, 2017-03-10 06:51:32]

Committed patchset #10 (id:170001) as
https://chromium.googlesource.com/chromium/src/+/05c00d2035047fa9d210d95afedf...

[kinuko, 2017-03-10 09:45:26]

A revert of this CL (patchset #10 id:170001) has been created in
https://codereview.chromium.org/2743733004/ by kinuko@chromium.org.

The reason for reverting is: Speculative revert, it doesn't look related but am
trying changes in the blame range.

Will update if the revert has changed things / will revert the revert otherwise.

https://build.chromium.org/p/chromium.win/builders/Win10%20Tests%20x64/builds...

Test:
SitePerProcessHighDPIExpiredCertBrowserTest.InterstitialLoadsWithCorrectDeviceScaleFactor.

[kinuko, 2017-03-10 10:34:16]

On 2017/03/10 09:45:26, kinuko wrote:
> A revert of this CL (patchset #10 id:170001) has been created in
> https://codereview.chromium.org/2743733004/ by
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=kinuko@chromium.org.
> 
> The reason for reverting is: Speculative revert, it doesn't look related but
am
> trying changes in the blame range.
> 
> Will update if the revert has changed things / will revert the revert
otherwise.
> 
>
https://build.chromium.org/p/chromium.win/builders/Win10%20Tests%20x64/builds...
> 
> Test:
>
SitePerProcessHighDPIExpiredCertBrowserTest.InterstitialLoadsWithCorrectDeviceScaleFactor.

Ok, I'm reverting this revert. Sorry for disturbing!