Closed Bug 1471062 Opened Last year Closed 5 months ago

Share more auxiliary data in JSScript

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [overhead:20k])

Attachments

(7 files, 1 obsolete file)

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.
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
Assignee: nobody → tcampbell
Ted, do you have an estimate of how much savings this would have?
Flags: needinfo?(tcampbell)
Whiteboard: [overhead:?]
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.
Flags: needinfo?(tcampbell)
Whiteboard: [overhead:?] → [overhead:20k]
Priority: -- → P3
See Also: → 1485347
Depends on: 1525924
Blocks: 1535138

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

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

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

Depends on D34789

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).

Depends on: 1558801
Attachment #9071747 - Attachment is obsolete: true
Attachment #9071748 - Attachment description: Bug 1471062 - Add SharedScriptData::offsetToPointer → Bug 1471062 - Add SharedScriptData::offsetToPointer. r?jandem
Attachment #9071749 - Attachment description: Bug 1471062 - Pad source notes to target alignment → Bug 1471062 - Pad source notes to target alignment. r?jandem
Attachment #9071751 - Attachment description: Bug 1471062 - Move resumeOffsets/scopeNotes/tryNotes to SharedScriptData → Bug 1471062 - Move resumeOffsets/scopeNotes/tryNotes to SharedScriptData. r?jandem

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

Depends on D34790

Attachment #9071750 - Attachment description: Bug 1471062 - Add SharedScriptData::flags → Bug 1471062 - Add SharedScriptData::flags. r?jandem
Attachment #9071752 - Attachment description: Bug 1471062 - Butterfly memory layout for SharedScriptData → Bug 1471062 - Avoid storing SharedScriptData metadata for empty arrays. r?jandem

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.

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.

Pushed by tcampbell@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.