1865502 - Align hash fields in all atom subtypes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

[Steve Fink [:sfink] [:s:]]

From Doug & Iain, it looks like it would help some to align the hash_ member of JSAtom subclasses to avoid having to branch on the flags. That does mean shrinking fat inline atoms down from 40 -> 32 bytes on 64-bit, which would ordinarily mean losing 8 bytes of inline space (so 24->16 bytes). But the hash value is only 32 bits and it can be aligned at the end, so this only loses 4 bytes instead (24->20 max inline bytes on 64-bit). Not only that, but this makes thin and fat inline atoms be the same size, so we can merge them (or discard one, depending on how you want to think about it.)

This does add some complexity because now there are 3 inline string length limits (per CharT). This caused less trouble than I expected, because most callers already know whether they're dealing with strings or atoms.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1865502 - Split out a ThinInlineAtom class

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1865502 - sizeof(JSAtom) does not need to match sizeof(JSString)

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1865502 - Pull out `JSAtom::lengthFitsInline<CharT>()` to ask "will this fit in any inline atom?"

Note that this is temporary, and will be removed later in this stack after there is only one type of inline atom.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1865502 - Shrink FatInlineAtom by 8 bytes on 64-bit

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1865502 - Merge the now-redundant FatInlineAtom + ThinInlineAtom -> InlineAtom

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1865502 - Move hash_ field into base JSAtom class

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1865502 - Recover 4 inline atom bytes by aligning the hash value with the end of the word

Comment 8

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1865502 - Add inline atom sizes to getBuildConfiguration()

Comment 9

[Jan de Mooij [:jandem]]

This is great. Thanks for looking into this!

Comment 10

[Steve Fink [:sfink] [:s:]]

Status: it seems to fully work on 64-bit, so I started looking at 32-bit. Sadly, I don't think we can do the same thing on 32-bit. At best, we can gain back the 4 bytes from inline atoms, but we can't have a single offset for all hash values (and that means I need to bring back all the JIT code I deleted, and just make it 32 bit only.)

I do have an idea for speeding up the 32-bit hash load very slightly, but I'll try that out after settling the rest of this. It may require some reworking of flag bits, and I'm not 100% sure it'll work. I'll file the bug in case someone else wants to tell me why it's stupid: bug 1865900.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/005d4a0ae4b4
Split out a ThinInlineAtom class r=jandem
https://hg.mozilla.org/integration/autoland/rev/15de8d55fe36
sizeof(JSAtom) does not need to match sizeof(JSString) r=jandem
https://hg.mozilla.org/integration/autoland/rev/c61c8700f220
Pull out `JSAtom::lengthFitsInline<CharT>()` to ask "will this fit in any inline atom?" r=jandem
https://hg.mozilla.org/integration/autoland/rev/1144485344ae
Shrink FatInlineAtom by 8 bytes on 64-bit r=jandem
https://hg.mozilla.org/integration/autoland/rev/912abfedeb09
Recover 4 inline atom bytes by aligning the hash value with the end of the word r=jandem
https://hg.mozilla.org/integration/autoland/rev/0c74789d4118
Add inline atom sizes to getBuildConfiguration() r=jandem

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/005d4a0ae4b4
https://hg.mozilla.org/mozilla-central/rev/15de8d55fe36
https://hg.mozilla.org/mozilla-central/rev/c61c8700f220
https://hg.mozilla.org/mozilla-central/rev/1144485344ae
https://hg.mozilla.org/mozilla-central/rev/912abfedeb09
https://hg.mozilla.org/mozilla-central/rev/0c74789d4118