1471878 - Cells should use alignas to enforce size and alignment requirements

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P5)

Description

[Jon Coppeard (:jonco)]

At the moment we manually pad cells like this:

https://searchfox.org/mozilla-central/source/js/src/vm/JSScript.h#1183

We should be able to give Cell an alignas specifier and have the compiler enforce this for us.

Comment 1

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1471878-Use-alignas-on-gc-Cell-instead-of-manual.patch

Use alignas on gc::Cell and remove manual padding.

Comment 2

[Ted Campbell [:tcampbell]]

Try Run in progress:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5830773d1029b368fea0e84d7a0abc2dafdd22b&group_state=expanded

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 8997022 [details] [diff] [review]
0001-Bug-1471878-Use-alignas-on-gc-Cell-instead-of-manual.patch

Review of attachment 8997022 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad5ea44b2053
Use alignas on gc::Cell instead of manual padding. r=jonco

Comment 5

[Kris Maglione [:kmag]]

This caused a 17K improvement on base content JS usage \o/

Comment 6

[Ted Campbell [:tcampbell]]

Wut. I didn't expect any changes. This makes me wonder what is happening..

Comment 7

[Kris Maglione [:kmag]]

(In reply to Ted Campbell [:tcampbell] from comment #6)
> Wut. I didn't expect any changes. This makes me wonder what is happening..

Heh. Interesting. Well, it's a pretty clear drop, and it stays that way:

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-inbound&originalRevision=32f2da1d88249ed0dd101a028187f7c9a20cdeb5&newProject=mozilla-inbound&newRevision=ad5ea44b20539d35847c031bbaa16367a66343f9&originalSignature=59b0a2b1333493edaf0312e02b03120d4a30a721&newSignature=59b0a2b1333493edaf0312e02b03120d4a30a721&framework=4

https://treeherder.mozilla.org/perf.html#/graphs?timerange=172800&series=mozilla-inbound,1684802,1,4&highlightedRevisions=ad5ea44b2053&zoom=1533221096513.5393,1533234233254.1567,6200000,6230337.078651685

Comment 8

[Ted Campbell [:tcampbell]]

Ah, It looks like it builds with VTUNE support and our packing of JSScript was silly before

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/ad5ea44b2053