Closed Bug 1568245 Opened 4 months ago Closed 2 months ago

Unify JSScript::realm_ / LazyScript::function_

Categories

(Core :: JavaScript Engine, task, P2)

task

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

References

(Blocks 1 open bug)

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.

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.

(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?

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.

The realm can be read off the global object and we can remove one step
in the mergeRealms code.

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

Attachment #9082779 - Attachment description: Bug 1568245 - Replace JSScript::realm_ with JSScript::global. r?jandem → Bug 1568245 - Replace JSScript::realm with JSScript::global. r?jandem
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

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.

Flags: needinfo?(tcampbell)
Depends on: 1575350
Depends on: 1578730

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

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
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Regressions: 1583860
Regressions: 1584138
You need to log in before you can comment on or make changes to this bug.