Move PreviousBoxGeometries from BoxPaintInvalidator into LayoutBox::m_rareData

Move PreviousBoxGeometries from BoxPaintInvalidator into LayoutBox::m_rareData

After https://codereview.chromium.org/2689213013/ and
https://codereview.chromium.org/2701103003/, now only 0.36% of
LayoutBoxes need PreviousBoxGeometries. Move it into
LayoutBox::m_rareData to simplify code and avoid map accesses.

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

Review-Url: https://codereview.chromium.org/2707073002
Cr-Commit-Position: refs/heads/master@{#452337}
Committed: https://chromium.googlesource.com/chromium/src/+/53e0e51e294b7b10dfaa31a5f1c99b527ab1f1c5

[Xianzhu, 2017-02-21 17:33:49]

Description was changed from

==========
Move PreviousBoxGeometries from BoxPaintInvalidator into LayoutBox::m_rareData

After https://codereview.chromium.org/2689213013/ and
https://codereview.chromium.org/2701103003/, now only 0.36% of
LayoutBoxes need PreviousBoxGeometries. Move it into
LayoutBox::m_rareData to simplify code and avoid map accesses.
==========

to

==========
Move PreviousBoxGeometries from BoxPaintInvalidator into LayoutBox::m_rareData

After https://codereview.chromium.org/2689213013/ and
https://codereview.chromium.org/2701103003/, now only 0.36% of
LayoutBoxes need PreviousBoxGeometries. Move it into
LayoutBox::m_rareData to simplify code and avoid map accesses.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Xianzhu, 2017-02-21 17:34:09]

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

[Xianzhu, 2017-02-21 17:34:24]

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

[Xianzhu, 2017-02-21 17:34:25]

This is the second followup of
https://codereview.chromium.org/2689213013/.

[commit-bot: I haz the power, 2017-02-21 17:35:00]

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 17:51:15]

Description was changed from

==========
Move PreviousBoxGeometries from BoxPaintInvalidator into LayoutBox::m_rareData

After https://codereview.chromium.org/2689213013/ and
https://codereview.chromium.org/2701103003/, now only 0.36% of
LayoutBoxes need PreviousBoxGeometries. Move it into
LayoutBox::m_rareData to simplify code and avoid map accesses.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Move PreviousBoxGeometries from BoxPaintInvalidator into LayoutBox::m_rareData

After https://codereview.chromium.org/2689213013/ and
https://codereview.chromium.org/2701103003/, now only 0.36% of
LayoutBoxes need PreviousBoxGeometries. Move it into
LayoutBox::m_rareData to simplify code and avoid map accesses.

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

[mstensho (USE GERRIT), 2017-02-21 17:51:51]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2017-02-21 17:51:51]

https://codereview.chromium.org/2707073002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2707073002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.h:1322: void
savePreviousOtherBoxGeometries() {
Pretty unusual to inline the rareData setters. Could you move it to .cpp?

[Xianzhu, 2017-02-21 18:14:48]

https://codereview.chromium.org/2707073002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2707073002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.h:1322: void
savePreviousOtherBoxGeometries() {
On 2017/02/21 17:51:51, mstensho wrote:
> Pretty unusual to inline the rareData setters. Could you move it to .cpp?

Done.

[mstensho (USE GERRIT), 2017-02-21 20:16:52]

https://codereview.chromium.org/2707073002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2707073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:1566: void
savePreviousOtherBoxGeometries();
Wouldn't it be better to keep this logic in MutableForPainting (i.e. make
LayoutBox::MutableForPainting::savePreviousOtherBoxGeometries() non-inline and
put the logic there), since all the getters are implemented there?

Alternatively, provide the getters here as well?

Keeping it all in MutableForPainting sounds more reasonable to me, if this is
only going to be used by the paint code. But as long as it's consistent, I'm
fine.

[Xianzhu, 2017-02-21 20:31:36]

https://codereview.chromium.org/2707073002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2707073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:1566: void
savePreviousOtherBoxGeometries();
On 2017/02/21 20:16:51, mstensho wrote:
> Wouldn't it be better to keep this logic in MutableForPainting (i.e. make
> LayoutBox::MutableForPainting::savePreviousOtherBoxGeometries() non-inline and
> put the logic there), since all the getters are implemented there?
> 
> Alternatively, provide the getters here as well?
> 
> Keeping it all in MutableForPainting sounds more reasonable to me, if this is
> only going to be used by the paint code. But as long as it's consistent, I'm
> fine.

The getters are not in MutableForPainting. They are public methods of LayoutBox,
just below MutableForPainting :)

savePreviousOtherBoxGeometries() was private here just for MutableForPainting.
Making MutableForPainting::savePreviousOtherBoxGeometries() non-inline looks
better. Done.

[mstensho (USE GERRIT), 2017-02-21 21:13:09]

core/layout/ lgtm.

I guess you want pdr to look at the changes in core/paint/

https://codereview.chromium.org/2707073002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2707073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:1566: void
savePreviousOtherBoxGeometries();
On 2017/02/21 20:31:35, Xianzhu wrote:
> On 2017/02/21 20:16:51, mstensho wrote:
> > Wouldn't it be better to keep this logic in MutableForPainting (i.e. make
> > LayoutBox::MutableForPainting::savePreviousOtherBoxGeometries() non-inline
and
> > put the logic there), since all the getters are implemented there?
> > 
> > Alternatively, provide the getters here as well?
> > 
> > Keeping it all in MutableForPainting sounds more reasonable to me, if this
is
> > only going to be used by the paint code. But as long as it's consistent, I'm
> > fine.
> 
> The getters are not in MutableForPainting. They are public methods of
LayoutBox,
> just below MutableForPainting :)

Yikes! I really though I had got used to the enforced new hideous coding style
by now, but that's apparently not the case. Sorry about that.

> savePreviousOtherBoxGeometries() was private here just for MutableForPainting.
> Making MutableForPainting::savePreviousOtherBoxGeometries() non-inline looks
> better. Done.

Yeah, this looks better. Thanks.

[pdr., 2017-02-22 18:37:54]

I like this, just some nits about names.

https://codereview.chromium.org/2707073002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2707073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:82: bool
m_hasPreviousOtherBoxGeometries : 1;
I think we have some free bits. WDYT about just splitting this out?
bool m_hasPreviousContentBoxSize : 1
bool m_hasPreviousLayoutOverflowRect : 1

Another alternative would be:
bool m_hasPreviousContentBoxSizeOrLayoutOverflowRect

[Xianzhu, 2017-02-22 18:58:52]

https://codereview.chromium.org/2707073002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2707073002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBox.h:82: bool
m_hasPreviousOtherBoxGeometries : 1;
On 2017/02/22 18:37:54, pdr. wrote:
> I think we have some free bits. WDYT about just splitting this out?
> bool m_hasPreviousContentBoxSize : 1
> bool m_hasPreviousLayoutOverflowRect : 1
> 
> Another alternative would be:
> bool m_hasPreviousContentBoxSizeOrLayoutOverflowRect

I choose m_hasPreviousContentBoxSizeAndLayoutOverflowRect. When we need to save
either of content box size and layout overflow rect, we can just save both
because we'll definitely allocate m_rareData.

Done.

(We could also save both whenever m_rareData is not null even if no need to
save, but that would make the interface between BoxPaintInvalidator and
LayoutBox complicated, and we must inline the saveXXX function because it would
be hot.)

[pdr., 2017-02-22 20:57:18]

On 2017/02/22 at 18:58:52, wangxianzhu wrote:
>
https://codereview.chromium.org/2707073002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> 
>
https://codereview.chromium.org/2707073002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBox.h:82: bool
m_hasPreviousOtherBoxGeometries : 1;
> On 2017/02/22 18:37:54, pdr. wrote:
> > I think we have some free bits. WDYT about just splitting this out?
> > bool m_hasPreviousContentBoxSize : 1
> > bool m_hasPreviousLayoutOverflowRect : 1
> > 
> > Another alternative would be:
> > bool m_hasPreviousContentBoxSizeOrLayoutOverflowRect
> 
> I choose m_hasPreviousContentBoxSizeAndLayoutOverflowRect. When we need to
save either of content box size and layout overflow rect, we can just save both
because we'll definitely allocate m_rareData.
> 
> Done.
> 
> (We could also save both whenever m_rareData is not null even if no need to
save, but that would make the interface between BoxPaintInvalidator and
LayoutBox complicated, and we must inline the saveXXX function because it would
be hot.)

LGTM

[Xianzhu, 2017-02-22 23:48:27]

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

[commit-bot: I haz the power, 2017-02-22 23:49:54]

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-22 23:57:26]

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

[commit-bot: I haz the power, 2017-02-22 23:57:27]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Xianzhu, 2017-02-23 00:02:16]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-02-23 00:02:17]

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

[commit-bot: I haz the power, 2017-02-23 00:04:08]

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-23 01:46:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487808136916750,
"parent_rev": "e23659b47bf9f47507986faba38b70aa10ca8497", "commit_rev":
"53e0e51e294b7b10dfaa31a5f1c99b527ab1f1c5"}

[commit-bot: I haz the power, 2017-02-23 01:47:26]

Description was changed from

==========
Move PreviousBoxGeometries from BoxPaintInvalidator into LayoutBox::m_rareData

After https://codereview.chromium.org/2689213013/ and
https://codereview.chromium.org/2701103003/, now only 0.36% of
LayoutBoxes need PreviousBoxGeometries. Move it into
LayoutBox::m_rareData to simplify code and avoid map accesses.

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

to

==========
Move PreviousBoxGeometries from BoxPaintInvalidator into LayoutBox::m_rareData

After https://codereview.chromium.org/2689213013/ and
https://codereview.chromium.org/2701103003/, now only 0.36% of
LayoutBoxes need PreviousBoxGeometries. Move it into
LayoutBox::m_rareData to simplify code and avoid map accesses.

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

Review-Url: https://codereview.chromium.org/2707073002
Cr-Commit-Position: refs/heads/master@{#452337}
Committed:
https://chromium.googlesource.com/chromium/src/+/53e0e51e294b7b10dfaa31a5f1c9...
==========

[commit-bot: I haz the power, 2017-02-23 01:47:27]

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