Move previous border box size out of global map into LayoutBox::m_previousSize

Move previous border box size out of global map into LayoutBox::m_previousSize

This slightly increases size of LayoutBox by 8 bytes, but reduces global
map accesses and simplifies code.

Here are data in top 10k sites collected on ct.skia.org:
- % of LayoutBoxes that needed to save previousBorderBoxSize: 22.8%
- % of LayoutBoxes that needed to save previousContentBoxRect because of box-sizing: 12.0%
- % of LayoutBoxes that needed to save previousContentBoxRect or previousLayoutOverflowRect because of background: 0.36%

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2689213013
Cr-Commit-Position: refs/heads/master@{#451506}
Committed: https://chromium.googlesource.com/chromium/src/+/2d744514083dfee8ea2bdde294738e3943a7a02e

[Xianzhu, 2017-02-17 00:18:16]

Description was changed from

==========
Move previous border box size out of global map into LayoutBox::m_previousSize

This slightly increases size of LayoutBox by 8 bytes, but avoids global
map access to previousBorderBoxSize and simplifies code.

Also changed previousContentBoxRect to previousContentBoxSize and save
it only when it's different from the border box size.
==========

to

==========
Move previous border box size out of global map into LayoutBox::m_previousSize

This slightly increases size of LayoutBox by 8 bytes, but avoids global
map access to previousBorderBoxSize and simplifies code.

Also changed previousContentBoxRect to previousContentBoxSize and save
it only when it's different from the border box size.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

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

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

[commit-bot: I haz the power, 2017-02-17 00:19:38]

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-17 02:24:20]

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

[commit-bot: I haz the power, 2017-02-17 02:24:20]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_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_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Xianzhu, 2017-02-17 19:39:38]

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

[commit-bot: I haz the power, 2017-02-17 19:40:49]

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-17 20:38:28]

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

[commit-bot: I haz the power, 2017-02-17 20:38:29]

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-17 21:58:53]

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

[commit-bot: I haz the power, 2017-02-17 21:59:55]

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-17 22:30:49]

Description was changed from

==========
Move previous border box size out of global map into LayoutBox::m_previousSize

This slightly increases size of LayoutBox by 8 bytes, but avoids global
map access to previousBorderBoxSize and simplifies code.

Also changed previousContentBoxRect to previousContentBoxSize and save
it only when it's different from the border box size.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Move previous border box size out of global map into LayoutBox::m_previousSize

This slightly increases size of LayoutBox by 8 bytes, but reduces global
map accesses and simplifies code.

Also save previousContentBoxRect and previousLayoutOverflowRect only if they
are different from the border box size.

Here are data in top 10k sites collected on ct.skia.org:
- % of LayoutBoxes that needed to save previousBorderBoxSize: 22.8%
- % of LayoutBoxes that needed to save previousContentBoxRect: 12.0%
- % of LayoutBoxes that needed to save previousLayoutOverflowRect: 0.36%

With this CL, previousSize is saved in LayoutBox. With the new condition
of previousContentBoxRect, we now save it for 4.7% of all LayoutBoxes.

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

[Xianzhu, 2017-02-17 22:31:11]

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

[pdr., 2017-02-17 23:07:19]

I'm on-board with this change (LGTM as-is), but wanted to see what you think
about a small change. If ~5% of boxes use this, could we put this in
LayoutBoxRareData? This approach also has downsides since ~5% of boxes would now
allocate a big rare data object, but it might be better in the common case.

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

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

[commit-bot: I haz the power, 2017-02-18 00:02:19]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_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_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Xianzhu, 2017-02-18 00:03:21]

On 2017/02/17 23:07:19, pdr. wrote:
> I'm on-board with this change (LGTM as-is), but wanted to see what you think
> about a small change. If ~5% of boxes use this, could we put this in
> LayoutBoxRareData? This approach also has downsides since ~5% of boxes would
now
> allocate a big rare data object, but it might be better in the common case.

We had been using rare data for these fields before moving paint invalidation
code from LayoutBox into BoxPaintInvalidator. I moved them into static global
map to minimize the interactions between LayoutBox and BoxPaintInvalidator, but
now I have realized this actually needs the bit guard and willBeDestroyed
notification which actually defeat the isolation.

Now I'm thinking of rareData might be better. Will collect more data to support
this.

Current data is:
- about 5% PreviousBoxGeometries
- about 3% of LayoutBoxes having m_rareData.
We need to know how much they overlap.

I'm also thinking of reverting the padding part of
https://codereview.chromium.org/340323002, so that we don't need the
box-sizing:border-box special case. This will simplify code, and I guess this
will reduce the needs of PreviousBoxGeometries to less than 1%. The case that
padding change without needing paint invalidation looks so special and it seems
not worth to support it.

[pdr., 2017-02-18 00:10:35]

On 2017/02/18 at 00:03:21, wangxianzhu wrote:
> On 2017/02/17 23:07:19, pdr. wrote:
> > I'm on-board with this change (LGTM as-is), but wanted to see what you think
> > about a small change. If ~5% of boxes use this, could we put this in
> > LayoutBoxRareData? This approach also has downsides since ~5% of boxes would
now
> > allocate a big rare data object, but it might be better in the common case.
> 
> We had been using rare data for these fields before moving paint invalidation
code from LayoutBox into BoxPaintInvalidator. I moved them into static global
map to minimize the interactions between LayoutBox and BoxPaintInvalidator, but
now I have realized this actually needs the bit guard and willBeDestroyed
notification which actually defeat the isolation.

Can you explain this a little more? What do you mean by "the bit guard and
willBeDestroyed notification"?

> 
> Now I'm thinking of rareData might be better. Will collect more data to
support this.
> 
> Current data is:
> - about 5% PreviousBoxGeometries
> - about 3% of LayoutBoxes having m_rareData.
> We need to know how much they overlap.
> 
> I'm also thinking of reverting the padding part of
https://codereview.chromium.org/340323002, so that we don't need the
box-sizing:border-box special case. This will simplify code, and I guess this
will reduce the needs of PreviousBoxGeometries to less than 1%. The case that
padding change without needing paint invalidation looks so special and it seems
not worth to support it.

[Xianzhu, 2017-02-18 00:24:25]

On 2017/02/18 00:10:35, pdr. wrote:
> On 2017/02/18 at 00:03:21, wangxianzhu wrote:
> > On 2017/02/17 23:07:19, pdr. wrote:
> > > I'm on-board with this change (LGTM as-is), but wanted to see what you
think
> > > about a small change. If ~5% of boxes use this, could we put this in
> > > LayoutBoxRareData? This approach also has downsides since ~5% of boxes
would
> now
> > > allocate a big rare data object, but it might be better in the common
case.
> > 
> > We had been using rare data for these fields before moving paint
invalidation
> code from LayoutBox into BoxPaintInvalidator. I moved them into static global
> map to minimize the interactions between LayoutBox and BoxPaintInvalidator,
but
> now I have realized this actually needs the bit guard and willBeDestroyed
> notification which actually defeat the isolation.
> 
> Can you explain this a little more? What do you mean by "the bit guard and
> willBeDestroyed notification"?

I moved the fields out of LayoutBox::m_rareData into the global map in
BoxPaintInvalidator to avoid interfaces between LayoutBox and
BoxPaintInvalidator like LayoutBox::previousXXX() and
LayoutBox::setPreviousXXX(), but that needs new interfaces like
LayoutObject::hasPreviousBoxGeometries() etc, and LayoutBox needs to notify
BoxPaintInvalidator to remove the box from the global map in
LayoutBox::willBeDestroyed(), so the there seems no reduction of interactions.

[Xianzhu, 2017-02-18 00:31:10]

I would like to land this patch as-is, and follow-up:

1. Revert the change about padding style change in
https://codereview.chromium.org/340323002, and remove the special handling about
content box change for box-sizing: border-box. This will reduce the needs of
PreviousBoxGeometry to 0.36% of all layout boxes. Just examined the stat and
found that the 0.36% data is actually the sum of percentage of LayoutBoxes
having background layers using content box or layout overflow box. Updated CL
description to reflect this.

2. Move PreviousBoxGeometry into LayoutBox::m_rareData.

Wdyt?

[Xianzhu, 2017-02-18 00:32:12]

Description was changed from

==========
Move previous border box size out of global map into LayoutBox::m_previousSize

This slightly increases size of LayoutBox by 8 bytes, but reduces global
map accesses and simplifies code.

Also save previousContentBoxRect and previousLayoutOverflowRect only if they
are different from the border box size.

Here are data in top 10k sites collected on ct.skia.org:
- % of LayoutBoxes that needed to save previousBorderBoxSize: 22.8%
- % of LayoutBoxes that needed to save previousContentBoxRect: 12.0%
- % of LayoutBoxes that needed to save previousLayoutOverflowRect: 0.36%

With this CL, previousSize is saved in LayoutBox. With the new condition
of previousContentBoxRect, we now save it for 4.7% of all LayoutBoxes.

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

to

==========
Move previous border box size out of global map into LayoutBox::m_previousSize

This slightly increases size of LayoutBox by 8 bytes, but reduces global
map accesses and simplifies code.

Also save previousContentBoxRect and previousLayoutOverflowRect only if they
are different from the border box size.

Here are data in top 10k sites collected on ct.skia.org:
- % of LayoutBoxes that needed to save previousBorderBoxSize: 22.8%
- % of LayoutBoxes that needed to save previousContentBoxRect because of
box-sizing: 12.0%
- % of LayoutBoxes that needed to save previousContentBoxRect or
previousLayoutOverflowRect because of background: 0.36%

With this CL, previousSize is saved in LayoutBox. With the new condition
of previousContentBoxRect, we now save it for 4.7% of all LayoutBoxes.

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

[Xianzhu, 2017-02-18 00:40:48]

Description was changed from

==========
Move previous border box size out of global map into LayoutBox::m_previousSize

This slightly increases size of LayoutBox by 8 bytes, but reduces global
map accesses and simplifies code.

Also save previousContentBoxRect and previousLayoutOverflowRect only if they
are different from the border box size.

Here are data in top 10k sites collected on ct.skia.org:
- % of LayoutBoxes that needed to save previousBorderBoxSize: 22.8%
- % of LayoutBoxes that needed to save previousContentBoxRect because of
box-sizing: 12.0%
- % of LayoutBoxes that needed to save previousContentBoxRect or
previousLayoutOverflowRect because of background: 0.36%

With this CL, previousSize is saved in LayoutBox. With the new condition
of previousContentBoxRect, we now save it for 4.7% of all LayoutBoxes.

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

to

==========
Move previous border box size out of global map into LayoutBox::m_previousSize

This slightly increases size of LayoutBox by 8 bytes, but reduces global
map accesses and simplifies code.

Here are data in top 10k sites collected on ct.skia.org:
- % of LayoutBoxes that needed to save previousBorderBoxSize: 22.8%
- % of LayoutBoxes that needed to save previousContentBoxRect because of
box-sizing: 12.0%
- % of LayoutBoxes that needed to save previousContentBoxRect or
previousLayoutOverflowRect because of background: 0.36%

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

[Xianzhu, 2017-02-18 00:40:57]

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

[commit-bot: I haz the power, 2017-02-18 00:41:45]

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-18 02:43:31]

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

[commit-bot: I haz the power, 2017-02-18 02:43:32]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_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_asan_rel_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)

[pdr., 2017-02-18 04:05:17]

On 2017/02/18 at 00:31:11, wangxianzhu wrote:
> I would like to land this patch as-is, and follow-up:
> 
> 1. Revert the change about padding style change in
https://codereview.chromium.org/340323002, and remove the special handling about
content box change for box-sizing: border-box. This will reduce the needs of
PreviousBoxGeometry to 0.36% of all layout boxes. Just examined the stat and
found that the 0.36% data is actually the sum of percentage of LayoutBoxes
having background layers using content box or layout overflow box. Updated CL
description to reflect this.
> 
> 2. Move PreviousBoxGeometry into LayoutBox::m_rareData.
> 
> Wdyt?

I like this plan, LGTM.

[Xianzhu, 2017-02-18 18:27:40]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-02-18 18:27: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-02-18 19:15:50]

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

[commit-bot: I haz the power, 2017-02-18 19:15:51]

Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[Xianzhu, 2017-02-18 23:20:50]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-02-18 23:20:51]

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

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

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-19 01:03:59]

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

[commit-bot: I haz the power, 2017-02-19 01:04:00]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Xianzhu, 2017-02-19 01:15:04]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-02-19 01:15:05]

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

[commit-bot: I haz the power, 2017-02-19 01:15:10]

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-19 02:55:36]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487466904905590,
"parent_rev": "f7c7217dd882eac40d5d07e441db5ef9fe3c94a4", "commit_rev":
"2d744514083dfee8ea2bdde294738e3943a7a02e"}

[commit-bot: I haz the power, 2017-02-19 02:56:31]

Description was changed from

==========
Move previous border box size out of global map into LayoutBox::m_previousSize

This slightly increases size of LayoutBox by 8 bytes, but reduces global
map accesses and simplifies code.

Here are data in top 10k sites collected on ct.skia.org:
- % of LayoutBoxes that needed to save previousBorderBoxSize: 22.8%
- % of LayoutBoxes that needed to save previousContentBoxRect because of
box-sizing: 12.0%
- % of LayoutBoxes that needed to save previousContentBoxRect or
previousLayoutOverflowRect because of background: 0.36%

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

to

==========
Move previous border box size out of global map into LayoutBox::m_previousSize

This slightly increases size of LayoutBox by 8 bytes, but reduces global
map accesses and simplifies code.

Here are data in top 10k sites collected on ct.skia.org:
- % of LayoutBoxes that needed to save previousBorderBoxSize: 22.8%
- % of LayoutBoxes that needed to save previousContentBoxRect because of
box-sizing: 12.0%
- % of LayoutBoxes that needed to save previousContentBoxRect or
previousLayoutOverflowRect because of background: 0.36%

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2689213013
Cr-Commit-Position: refs/heads/master@{#451506}
Committed:
https://chromium.googlesource.com/chromium/src/+/2d744514083dfee8ea2bdde29473...
==========

[commit-bot: I haz the power, 2017-02-19 02:56:32]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/2d744514083dfee8ea2bdde29473...