Reduce VC++ padding in IncrementalMarking class

Reduce VC++ padding in IncrementalMarking class

IncrementalMarking has nine bytes of padding in 32-bit and 64-bit
builds. Fixing 32-bit builds just requires moving the one-byte
incremental_marking_job_ member. Fixing 64-bit requires moving the
four-byte state_ member. This change reduces the padding to one byte.

On 64-bit this reduces its size from 152 to 144. This also fits heap
granularity better. On 32-bit it goes from 96 to 88 bytes.

The initial padding was found with llvm-pdbdump.exe. The fix was
verified by compiling v8/src/assembler.cc with the undocumented
/d1reportSingleClassLayout option, like this:

    /d1reportSingleClassLayoutIncrementalMarking

The savings should apply on all platforms, or at worst should make no
difference except for improving alignment.

Thanks to zturner@ for some llvm-pdbdump improvements.

BUG=chromium:710933
Review-Url: https://codereview.chromium.org/2808473003
Cr-Commit-Position: refs/heads/master@{#44698}
Committed: https://chromium.googlesource.com/v8/v8/+/f4a57545e8b89fb2bbf23d6724d2654f368601eb

[brucedawson, 2017-04-07 21:28:08]

brucedawson@chromium.org changed reviewers:
+ bradnelson@chromium.org

[brucedawson, 2017-04-07 21:28:09]

I don't know if this object is allocated enough to matter. On 64-bit this
reduces its size from 152 to 144. This also fits heap granularity better. On
32-bit it goes from 96 to 88 bytes.

To some extent this was just a test of the techniques needed. If there are v8
classes which there are many instances of I can audit them for
alignment/padding.

[brucedawson, 2017-04-07 21:30:36]

Description was changed from

==========
Reduce VC++ padding in IncrementalMarking class

IncrementalMarking has nine bytes of padding in 32-bit and 64-bit
builds. Fixing 32-bit builds just requires moving the one-byte
incremental_marking_job_ member. Fixing 64-bit requires moving the
four-byte state_ member.

The initial padding was found with llvm-pdbdump.exe. The fix was
verified by compiling v8/src/assembler.cc with the undocumented
/d1reportSingleClassLayout option, like this:

    /d1reportSingleClassLayoutIncrementalMarking

The savings should apply on all platforms, or at worst should make no
difference except for improving alignment.

Thanks to zturner@ for some llvm-pdbdump improvements.
==========

to

==========
Reduce VC++ padding in IncrementalMarking class

IncrementalMarking has nine bytes of padding in 32-bit and 64-bit
builds. Fixing 32-bit builds just requires moving the one-byte
incremental_marking_job_ member. Fixing 64-bit requires moving the
four-byte state_ member. This change reduces the padding to one byte.

On 64-bit this reduces its size from 152 to 144. This also fits heap
granularity better. On 32-bit it goes from 96 to 88 bytes.

The initial padding was found with llvm-pdbdump.exe. The fix was
verified by compiling v8/src/assembler.cc with the undocumented
/d1reportSingleClassLayout option, like this:

    /d1reportSingleClassLayoutIncrementalMarking

The savings should apply on all platforms, or at worst should make no
difference except for improving alignment.

Thanks to zturner@ for some llvm-pdbdump improvements.
==========

[bradnelson, 2017-04-12 16:47:30]

lgtm
Any concerns hpayer?

[brucedawson, 2017-04-12 17:39:54]

Description was changed from

==========
Reduce VC++ padding in IncrementalMarking class

IncrementalMarking has nine bytes of padding in 32-bit and 64-bit
builds. Fixing 32-bit builds just requires moving the one-byte
incremental_marking_job_ member. Fixing 64-bit requires moving the
four-byte state_ member. This change reduces the padding to one byte.

On 64-bit this reduces its size from 152 to 144. This also fits heap
granularity better. On 32-bit it goes from 96 to 88 bytes.

The initial padding was found with llvm-pdbdump.exe. The fix was
verified by compiling v8/src/assembler.cc with the undocumented
/d1reportSingleClassLayout option, like this:

    /d1reportSingleClassLayoutIncrementalMarking

The savings should apply on all platforms, or at worst should make no
difference except for improving alignment.

Thanks to zturner@ for some llvm-pdbdump improvements.
==========

to

==========
Reduce VC++ padding in IncrementalMarking class

IncrementalMarking has nine bytes of padding in 32-bit and 64-bit
builds. Fixing 32-bit builds just requires moving the one-byte
incremental_marking_job_ member. Fixing 64-bit requires moving the
four-byte state_ member. This change reduces the padding to one byte.

On 64-bit this reduces its size from 152 to 144. This also fits heap
granularity better. On 32-bit it goes from 96 to 88 bytes.

The initial padding was found with llvm-pdbdump.exe. The fix was
verified by compiling v8/src/assembler.cc with the undocumented
/d1reportSingleClassLayout option, like this:

    /d1reportSingleClassLayoutIncrementalMarking

The savings should apply on all platforms, or at worst should make no
difference except for improving alignment.

Thanks to zturner@ for some llvm-pdbdump improvements.

BUG=710933
==========

[brucedawson, 2017-04-14 00:18:32]

The CQ bit was checked by brucedawson@chromium.org

[commit-bot: I haz the power, 2017-04-14 00:18:40]

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-04-14 00:21:53]

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

[commit-bot: I haz the power, 2017-04-14 00:21:54]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/39198)

[brucedawson, 2017-04-14 00:29:11]

Description was changed from

==========
Reduce VC++ padding in IncrementalMarking class

IncrementalMarking has nine bytes of padding in 32-bit and 64-bit
builds. Fixing 32-bit builds just requires moving the one-byte
incremental_marking_job_ member. Fixing 64-bit requires moving the
four-byte state_ member. This change reduces the padding to one byte.

On 64-bit this reduces its size from 152 to 144. This also fits heap
granularity better. On 32-bit it goes from 96 to 88 bytes.

The initial padding was found with llvm-pdbdump.exe. The fix was
verified by compiling v8/src/assembler.cc with the undocumented
/d1reportSingleClassLayout option, like this:

    /d1reportSingleClassLayoutIncrementalMarking

The savings should apply on all platforms, or at worst should make no
difference except for improving alignment.

Thanks to zturner@ for some llvm-pdbdump improvements.

BUG=710933
==========

to

==========
Reduce VC++ padding in IncrementalMarking class

IncrementalMarking has nine bytes of padding in 32-bit and 64-bit
builds. Fixing 32-bit builds just requires moving the one-byte
incremental_marking_job_ member. Fixing 64-bit requires moving the
four-byte state_ member. This change reduces the padding to one byte.

On 64-bit this reduces its size from 152 to 144. This also fits heap
granularity better. On 32-bit it goes from 96 to 88 bytes.

The initial padding was found with llvm-pdbdump.exe. The fix was
verified by compiling v8/src/assembler.cc with the undocumented
/d1reportSingleClassLayout option, like this:

    /d1reportSingleClassLayoutIncrementalMarking

The savings should apply on all platforms, or at worst should make no
difference except for improving alignment.

Thanks to zturner@ for some llvm-pdbdump improvements.

BUG=chromium:710933
==========

[brucedawson, 2017-04-14 00:29:11]

brucedawson@chromium.org changed reviewers:
+ hpayer@chromium.org

[brucedawson, 2017-04-14 00:29:25]

hpayer@ - PTAL?

[brucedawson, 2017-04-17 21:29:43]

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

[commit-bot: I haz the power, 2017-04-17 21:29:51]

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-04-17 21:56:46]

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

[commit-bot: I haz the power, 2017-04-17 21:57:03]

Dry run: This issue passed the CQ dry run.

[brucedawson, 2017-04-17 22:26:13]

Okay, it passes the try bots now. Previously the compiler was warning that the
constructor initialization order was inconsistent with the class order, and I
hadn't noticed the failures. I've fixed that now.

BTW, I guess this is another reason to like inline member initialization, since
then the two places don't need to be manually synchronized.

PTAL hpayer@

[Hannes Payer (out of office), 2017-04-18 16:10:19]

LGTM, sorry, was OOO.

It will not make a difference for V8 overall since these objects are not used a
lot. It may however make sense to look into other objects.

[brucedawson, 2017-04-18 18:10:19]

On 2017/04/18 16:10:19, Hannes Payer wrote:
> LGTM, sorry, was OOO.
> 
> It will not make a difference for V8 overall since these objects are not used
a
> lot. It may however make sense to look into other objects.

Understood. If you have any recommendations on what types or classes of types
would gain the most from having their packing optimized then please let me know
- perhaps add comments to bug 710933?

[brucedawson, 2017-04-18 18:10:23]

The CQ bit was checked by brucedawson@chromium.org

[brucedawson, 2017-04-18 18:10:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org
Link to the patchset: https://codereview.chromium.org/2808473003/#ps20001
(title: "Reorder initialization")

[commit-bot: I haz the power, 2017-04-18 18:10:35]

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-04-18 18:13:28]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492539023528350,
"parent_rev": "26bc5906295100300252463e2f5914279e30970f", "commit_rev":
"f4a57545e8b89fb2bbf23d6724d2654f368601eb"}

[commit-bot: I haz the power, 2017-04-18 18:13:36]

Description was changed from

==========
Reduce VC++ padding in IncrementalMarking class

IncrementalMarking has nine bytes of padding in 32-bit and 64-bit
builds. Fixing 32-bit builds just requires moving the one-byte
incremental_marking_job_ member. Fixing 64-bit requires moving the
four-byte state_ member. This change reduces the padding to one byte.

On 64-bit this reduces its size from 152 to 144. This also fits heap
granularity better. On 32-bit it goes from 96 to 88 bytes.

The initial padding was found with llvm-pdbdump.exe. The fix was
verified by compiling v8/src/assembler.cc with the undocumented
/d1reportSingleClassLayout option, like this:

    /d1reportSingleClassLayoutIncrementalMarking

The savings should apply on all platforms, or at worst should make no
difference except for improving alignment.

Thanks to zturner@ for some llvm-pdbdump improvements.

BUG=chromium:710933
==========

to

==========
Reduce VC++ padding in IncrementalMarking class

IncrementalMarking has nine bytes of padding in 32-bit and 64-bit
builds. Fixing 32-bit builds just requires moving the one-byte
incremental_marking_job_ member. Fixing 64-bit requires moving the
four-byte state_ member. This change reduces the padding to one byte.

On 64-bit this reduces its size from 152 to 144. This also fits heap
granularity better. On 32-bit it goes from 96 to 88 bytes.

The initial padding was found with llvm-pdbdump.exe. The fix was
verified by compiling v8/src/assembler.cc with the undocumented
/d1reportSingleClassLayout option, like this:

    /d1reportSingleClassLayoutIncrementalMarking

The savings should apply on all platforms, or at worst should make no
difference except for improving alignment.

Thanks to zturner@ for some llvm-pdbdump improvements.

BUG=chromium:710933

Review-Url: https://codereview.chromium.org/2808473003
Cr-Commit-Position: refs/heads/master@{#44698}
Committed:
https://chromium.googlesource.com/v8/v8/+/f4a57545e8b89fb2bbf23d6724d2654f368...
==========

[commit-bot: I haz the power, 2017-04-18 18:13:37]

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