Refactor XDRScript to better align with current JSScript struct

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P3
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: tcampbell, Assigned: tcampbell)

Tracking

(Blocks 1 bug)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(9 attachments)

js::XDRScript has envolved over the years into a complex mess as JSScript has changed. With Bug 1485347 cleaning up a lot of stuff in the data structure, it makes sense to apply a similar structure to XDRScript (and later CopyScript).

The motivation is to make XDR more robust and make it easy to change JSScript for content-overhead and generated-interpreter projects.

Blocks: 1308252

Depends on D19005

Also change JSTryNote::kind to uint32_t to absorb alignment padding of
structure.

Depends on D19006

Depends on D19007

Depends on D19008

Comment on attachment 9042185 [details]
Bug 1525924 - Part 6: Factor out PrivateScriptData::XDR

Nicolas, I'm rewriting XDRScript in this stack to better match our data structures. Any major objections or strong desire to review? Otherwise I've reviewed a lot of Jan's recent JSScript changes so I'll make him review these tedious cleanups.

Attachment #9042185 - Flags: feedback?

Comment on attachment 9042185 [details]
Bug 1525924 - Part 6: Factor out PrivateScriptData::XDR

(It works better when I include people's names... )
See Comment 7.

Attachment #9042185 - Flags: feedback? → feedback?(nicolas.b.pierron)
Depends on: 1526324

Make the method static so that in a follow-up the allocation of the
object can be absorbed in the method.

This moves the allocation of the ScriptSource object into
ScriptSource::XDR instead of being done in the caller. A
MutableHandle<ScriptSourceHolder> is used to manage both
ScriptSource::refCount as well as tracing Atoms from BinAST.

Depends on D19163

This makes XDRScript better follow the field layout of JSScript.

Blocks: 1471062

This series of changes cleans up XDRScript to a lot more organized and much better aligned with the current data structures. The next set of changes (in another bug) will probably change XDRScript/CopyScript/JSScript/etc at the same time to clarify initialization sequences to be less error-prone.

Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/34b6c82e60ce
Part 1: Factor out SharedScriptData::XDR r=jandem
https://hg.mozilla.org/integration/autoland/rev/582ab25b0a61
Part 2: Factor out WithScope::XDR r=jandem
https://hg.mozilla.org/integration/autoland/rev/496594078fd3
Part 3: Factor out JSTryNote/ScopeNote::XDR r=jandem
https://hg.mozilla.org/integration/autoland/rev/8045fef98288
Part 4: Factor out js::XDRScope r=jandem
https://hg.mozilla.org/integration/autoland/rev/3dde34aa913c
Part 5: Factor out js::XDRInnerObject r=jandem
https://hg.mozilla.org/integration/autoland/rev/99c5523b079b
Part 6: Factor out PrivateScriptData::XDR r=jandem
https://hg.mozilla.org/integration/autoland/rev/294323a1a8e6
Part 7: Make ScriptSource::performXDR a static method r=jandem
https://hg.mozilla.org/integration/autoland/rev/634ba2074e16
Part 8: Move allocation into ScriptSource::XDR r=jandem
https://hg.mozilla.org/integration/autoland/rev/df6a7d40a383
Part 9: Reorder steps of XDRScript r=jandem

Comment on attachment 9042185 [details]
Bug 1525924 - Part 6: Factor out PrivateScriptData::XDR

Nice refactoring! This would also make things easier to understand.

Attachment #9042185 - Flags: feedback?(nicolas.b.pierron) → feedback+
Regressions: 1545989
You need to log in before you can comment on or make changes to this bug.