Unify JSScript::realm_ / LazyScript::function_
Categories
(Core :: JavaScript Engine, task, P2)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox71 | --- | fixed |
People
(Reporter: tcampbell, Assigned: tcampbell)
References
Details
Attachments
(2 files)
When unifying LazyScript and JSScript, the JSScript::realm_ can be derived from the LazyScript::function_. This is a simplifies JSScript::function() access and makes delazify/relazify have fewer things to worry about.
For the case of top-level scripts, we can point to the GlobalObject so we have simply a GCPtrObject for both cases.
The realm is then accessed through obj->group_->realm_ which is two extra loads, but no other complexity.
|
Assignee | |
Comment 1•5 years ago
|
||
Question: Should module scripts point to the ModuleObject or is GlobalObject sufficient?
The primary use for explicitly having the JSFunction is LazyScripts don't have a function-scope to retrieve this from.
|
||
Comment 2•5 years ago
|
||
(In reply to Ted Campbell [:tcampbell] from comment #1)
Question: Should module scripts point to the ModuleObject or is GlobalObject sufficient?
GlobalObject is sufficient because LazyScript is never used for modules anyway, right?
|
|
Assignee | |
Comment 3•5 years ago
|
||
For the scope of this bug, I will add BaseScript::functionOrGlobal_. It will always be the global for non-lazy scripts. This unifies the definition of realm()/compartment() which is nice. In the future it might be nice to have function scripts always point to function and module scripts always point to module. This would be consistent with the body-scope associated object (FunctionScope and ModuleScope both have GCPtrObject). One day we may support re-lazifying modules scripts.
|
|
Assignee | |
Comment 4•5 years ago
|
||
The realm can be read off the global object and we can remove one step
in the mergeRealms code.
|
|
Assignee | |
Comment 5•5 years ago
|
||
Combine the LazyScript::function_ and JSScript::global_ fields into the
BaseScript type. This provides a common definition of script realm and
compartment. Currently a non-lazy function script will set this to point
this to the global, but in future it should be made to point to
canonical function for both the lazy and non-lazy cases.
Depends on D40519
|
||
Updated•5 years ago
|
Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/26a34510d053 Replace JSScript::realm with JSScript::global. r=jandem https://hg.mozilla.org/integration/autoland/rev/05c4ca358415 Move JSScript::global to BaseScript. r=jandem
|
||
Comment 7•5 years ago
|
||
Backed out 2 changesets for causing bustages and crashes.
Backout link: https://hg.mozilla.org/integration/autoland/rev/cb67d2ac1d73308a7a226bf15da5151da59a89fb
Failure logs:
-
Bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=260121344&repo=autoland, https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=260118223&repo=autoland&lineNumber=40861
-
Crash at application crashed [@ js::UninlinedIsCrossCompartmentWrapper(JSObject const*)] : https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=260123188&repo=autoland&lineNumber=1338
|
|
Assignee | |
Comment 8•5 years ago
|
||
The failure is best illustrated in the valgrind logs. With these patches, one can no longer access realm while cleaning up JSScript (due to GC sweep phase ordering). We currently use realm to look up auxiliary vtune and ccov data.
One option may be to change the sweep order of SCRIPT and OBJECT. I'll look into this.
|
|
Assignee | |
Comment 9•5 years ago
|
||
With Bug 1575350 and Bug 1578730 fixed, the JSScript::finalize method should no longer access JSScript::realm() and these original patches should just work. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=87e65175a842f71a963afd6082090c15ceffbfa2
|
||
Comment 10•5 years ago
|
||
Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/39ce2fcdbbff Replace JSScript::realm with JSScript::global. r=jandem https://hg.mozilla.org/integration/autoland/rev/e35199501a97 Move JSScript::global to BaseScript. r=jandem
|
||
Comment 11•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/39ce2fcdbbff
https://hg.mozilla.org/mozilla-central/rev/e35199501a97
Description
•