1471062 - Share more auxiliary data in JSScript

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Ted Campbell [:tcampbell]]

It looks like some fields in JSScript unshared data can be moved to SharedScriptData, such as:

JSScript::consts()
JSScript::trynotes()
JSScript::scopeNotes()
JSScript::yieldAndAwaitOffsets()

This is expected to be a win for cases where JSScripts are cloned into other compartments, as well as helping Fission efforts.

Comment 1

[Ted Campbell [:tcampbell]]

Additional TODO here:
- Rename the data_ / scriptData_ fields to just about anything else
- Replace TryNoteArray, etc. return types with mozilla::Span<const T>
- The way they handle offsets needs to be reconciled

Comment 2

[Eric Rahm [:erahm]]

Ted, do you have an estimate of how much savings this would have?

Comment 3

[Ted Campbell [:tcampbell]]

Guestimate based on some about:memory data is around 20k. I'm working on this anyways for other code cleanup endeavors of spidermonkey. This also leads into the bytecode-sharing work which has the bulk of savings.

Comment 4

[Ted Campbell [:tcampbell]]

Attachment: Bug 1471062 - Change type of JSScript::consts() to GCPtrBigInt

Comment 5

[Ted Campbell [:tcampbell]]

Attachment: Bug 1471062 - Add SharedScriptData::offsetToPointer. r?jandem

Depends on D34786

Comment 6

[Ted Campbell [:tcampbell]]

Attachment: Bug 1471062 - Pad source notes to target alignment. r?jandem

Pad the source notes with SRC_NULL such that the code and notes arrays
together maintain uint32_t alignment. This is will later be used to add
optional trailing arrays with uint32_t alignment. It is expected that
the allocators would have not been using these bytes so memory usage
should be neutral.

Depends on D34787

Comment 7

[Ted Campbell [:tcampbell]]

Attachment: Bug 1471062 - Add SharedScriptData::flags. r?jandem

Add a flag into the byte array area. It is inserted before code() so it
will be at a fixed offset once atoms are removed. This will be used to
store flags about optional tail arrays.

Depends on D34788

Comment 8

[Ted Campbell [:tcampbell]]

Attachment: Bug 1471062 - Move resumeOffsets/scopeNotes/tryNotes to SharedScriptData. r?jandem

These arrays don't have any pointers and so should be made shareable.

Depends on D34789

Comment 9

[Ted Campbell [:tcampbell]]

Attachment: Bug 1471062 - Avoid storing SharedScriptData metadata for empty arrays. r?jandem

Depends on D34790

Comment 10

[Ted Campbell [:tcampbell]]

Patches work. This does some excessively clever packing to optimize for fast access of commonly accessed data and avoiding storing sizes for empty arrays (some of which are common). I'm still uncertain if this is a good idea. We should measure how 'optional' some of these optional arrays and make their size/offset always defined if that can reduce complexity.

This conflicts with Bug 1558801 because I forgot I already wrote the same patch (sorry).

Comment 11

[Ted Campbell [:tcampbell]]

Attachment: Bug 1471062 - Remove the PrivateScriptData::PackedSpan mechanism. r?jandem

Now that PrivateScriptData contains a single array, the PackedSpan
mechanism for packing multiple trailing arrays can be removed.

Depends on D34790

Comment 12

[Ted Campbell [:tcampbell]]

I've cleaned up the code to a point it can be landed. There is currently a 4kB JS regression on awsy-base because of padding issue not recovering removed fields. This will get better after feature changes. The primary motivation for this patch set is to move more data into SharedScriptData for when self-hosting/chrome bytecode can be used directly from binaries/cache.

Comment 13

[Ted Campbell [:tcampbell]]

Attachment: Bug 1471062 - Make heap-analysis/byteSize-of-scripts jit-test more complex r?jandem!,arai!

As PrivateScriptData contains less data, we need to make this test have
more complicated functions so that the test is not sensistive to
allocator rounding. This also removes the binjs variant of test until
they next time they are regenerated.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83a5257788cb
Make heap-analysis/byteSize-of-scripts jit-test more complex r=jandem,arai
https://hg.mozilla.org/integration/autoland/rev/0964e2a0ed29
Add SharedScriptData::offsetToPointer. r=jandem
https://hg.mozilla.org/integration/autoland/rev/f6226246197b
Pad source notes to target alignment. r=jandem
https://hg.mozilla.org/integration/autoland/rev/8718427bcb0f
Move resumeOffsets/scopeNotes/tryNotes to SharedScriptData. r=jandem
https://hg.mozilla.org/integration/autoland/rev/161f46ffacb7
Remove the PrivateScriptData::PackedSpan mechanism. r=jandem
https://hg.mozilla.org/integration/autoland/rev/9624fc4c9e10
Add SharedScriptData::flags. r=jandem
https://hg.mozilla.org/integration/autoland/rev/6ded0846247c
Avoid storing SharedScriptData metadata for empty arrays. r=jandem

Comment 15

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/83a5257788cb
https://hg.mozilla.org/mozilla-central/rev/0964e2a0ed29
https://hg.mozilla.org/mozilla-central/rev/f6226246197b
https://hg.mozilla.org/mozilla-central/rev/8718427bcb0f
https://hg.mozilla.org/mozilla-central/rev/161f46ffacb7
https://hg.mozilla.org/mozilla-central/rev/9624fc4c9e10
https://hg.mozilla.org/mozilla-central/rev/6ded0846247c