1459127 - Store ScriptSourceObject reference into LazyScript inside LazyScript

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Tooru Fujisawa [:arai]]

Currently LazyScript doesn't have ScriptSourceObject reference if the enclosing script is also lazified.

this is problematic for bug 1434305, which is to store LazyScript reference to Debugger.Script.

There are so many Debugger.Script.source property accesses, when opening devtools.
and we have to get ScriptSourceObject reference for given script.

what I'm doing now in the patch there is, find ancestor script which has ScriptSourceObject, by looping over the all LazyScript instances.
because there's no direct reference from inner LazyScript to enclosing LazyScript, but only the reference from enclosing LazyScript to inner JSFunction, which has LazyScript reference.
this process takes so long time when there are many scripts.

If we can store the ScriptSourceObject reference, this can be done quickly.
(or, perhaps store the direct reference to enclosing LazyScript, so that we can reach the ancestor quickly)

Comment 1

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #0)
> (or, perhaps store the direct reference to enclosing LazyScript, so that we
> can reach the ancestor quickly)

this will also solve the situation for delazifying the script.
currently, delazification requires LazyScript to have ScriptSourceObject reference,
and I think it actually means that the enclosing script is also delazified:
https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/js/src/vm/JSFunction.cpp#1616

so, in that case I'm now delazifying the enclosing LazyScript recursively.
which also requires looping over all scripts, to find enclosing script.

Comment 2

[Tooru Fujisawa [:arai]]

so, what happening now is:
  * we're creating LazyScript instance inside Parser, without setting ScriptSourceObject reference [1]
  * we're creating ScriptSourceObject instance inside BytecodeEmitter [2]
  * we're setting LazyScript's ScriptSourceObject reference only when emitting the function node, inside BytecodeEmitter [3]

so, if the enclosing function's bytecode is not yet emitted (that means the enclosing function is also lazified), the ScriptSourceObject reference is null.

then, we're creating ScriptSourceObject instance before creating Parser instance [4]
that means the ScriptSourceObject's reference is already known at the point of parsing.
what we have to do is just tell Parser about the ScriptSourceObject object, and let it set the reference to all LazyScript.

I'll check if any kind of issue happens if LazyScript has ScriptSourceObject reference in such case.

[1] https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/js/src/frontend/Parser.cpp#2533-2537
[2] https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/js/src/frontend/BytecodeCompiler.cpp#190
[3] https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/js/src/frontend/BytecodeEmitter.cpp#7944
[4] https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/js/src/frontend/BytecodeCompiler.cpp#241

Comment 3

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #2)
>   * we're creating ScriptSourceObject instance inside BytecodeEmitter [2]

actually, BytecodeCompiler.

Then, there are several places that checks LazyScript::sourceObject(), to see if the enclosing script is already compiled or not.

I'm about to keep enclosingScope null, and add LazyScript::isEnclosingScriptLazy, which checks enclosingScope_, instead of sourceObject_;

https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/js/src/vm/JSScript.cpp#4346-4354
> /* static */ LazyScript*
> LazyScript::Create(JSContext* cx, HandleFunction fun,
>                    HandleScript script, HandleScope enclosingScope,
>                    HandleScriptSource sourceObject,
>                    uint64_t packedFields, uint32_t sourceStart, uint32_t sourceEnd,
>                    uint32_t toStringStart, uint32_t lineno, uint32_t column)
> {
> ...
>     // Set the enclosing scope and source object of the lazy function. These
>     // values should only be non-null if we have a non-lazy enclosing script.
>     // AddLazyFunctionsForCompartment relies on the source object being null
>     // if we're nested inside another lazy function.
>     MOZ_ASSERT(!!sourceObject == !!enclosingScope);
>     MOZ_ASSERT(!res->sourceObject());
>     MOZ_ASSERT(!res->enclosingScope());
>     if (sourceObject)
>         res->setEnclosingScopeAndSource(enclosingScope, sourceObject);

https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/js/src/vm/JSCompartment.cpp#1103
> static bool
> AddLazyFunctionsForCompartment(JSContext* cx, AutoObjectVector& lazyFunctions, AllocKind kind)
> {
>     // Find all live root lazy functions in the compartment: those which have a
>     // source object, indicating that they have a parent, and which do not have
>     // an uncompiled enclosing script. The last condition is so that we don't
>     // compile lazy scripts whose enclosing scripts failed to compile,
>     // indicating that the lazy script did not escape the script.
>     //
>     // Some LazyScripts have a non-null |JSScript* script| pointer. We still
>     // want to delazify in that case: this pointer is weak so the JSScript
>     // could be destroyed at the next GC.
> 
>     for (auto i = cx->zone()->cellIter<JSObject>(kind); !i.done(); i.next()) {
> ...
>         if (fun->isInterpretedLazy()) {
>             LazyScript* lazy = fun->lazyScriptOrNull();
>             if (lazy && lazy->sourceObject() && !lazy->hasUncompiledEnclosingScript()) {

Comment 4

[Tooru Fujisawa [:arai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2f6f374f8f595a74a2a5251f988be5b319ceb21&group_state=expanded&selectedJob=177072204

The remaining issue is XDR, where nullptr is passed as sourceObject in some case

https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/js/src/vm/JSScript.cpp#970

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Store ScriptSourceObject reference into LazyScript inside LazyScript.

* Changed LazyScript to require ScriptSourceObject in constructor.
  * Changed Parser to receive ScriptSourceObject from BytecodeCompiler or surrounding code (create if necessary [*1]),
    and pass it to LazyScript when syntax parsing the script
  * Changed LazyScript::setEnclosingScopeAndSource to LazyScript::setEnclosingScope,
    since ScriptSourceObject is already set
  * Changed XDR to require ScriptSourceObject when decoding LazyScript [*2]
  * Changed LazyScript::isEnclosingScriptLazy to check enclosingScope_

[*1] There are some places that creates Parser (Reflect.parse, BinAST test, isCompilableUnit functions, some testing functions),
     and in that case delazification won't happen.
     so we can just create and pass a dummy ScriptSourceObject there

[*2] as far as I understand, decoding LazyScript happens only inside decoding non-lazy script, and in that case ScriptSourceObject for non-lazy script is already known, so we just have to pass the known ScriptSourceObject to decoding function

Comment 6

[Jim Blandy :jimb]

Comment on attachment 8974284 [details] [diff] [review]
Store ScriptSourceObject reference into LazyScript inside LazyScript.

Review of attachment 8974284 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great - thank you very much. Some minor changes requested.

::: js/src/frontend/Parser.h
@@ +275,5 @@
>      UsedNameTracker& usedNames;
>  
>      ScriptSource*       ss;
>  
> +    RootedScriptSourceObject sourceObject;

We shouldn't have `RootedBlah` in a struct unless that struct has the MOZ_STACK_CLASS attribute. Is there a reason `ParserBase` doesn't have that? If not, let's add it.

::: js/src/vm/JSScript.cpp
@@ +4225,4 @@
>  }
>  
>  ScriptSourceObject*
>  LazyScript::sourceObject() const

It would be nice to make this function return a ScriptSourceObject&, now that all LazyScripts have one.

::: js/src/vm/JSScript.h
@@ +2158,5 @@
>      // start parsing.
>      uint32_t lineno_;
>      uint32_t column_;
>  
> +    LazyScript(JSFunction* fun, ScriptSourceObject* sourceObject,

Since sourceObject isn't allowed to be null, should it be ScriptSourceObject& ?

@@ +2167,5 @@
>      // Create a LazyScript without initializing the closedOverBindings and the
>      // innerFunctions. To be GC-safe, the caller must initialize both vectors
>      // with valid atoms and functions.
>      static LazyScript* CreateRaw(JSContext* cx, HandleFunction fun,
> +                                 HandleScriptSourceObject sourceObject,

Similarly

@@ +2178,5 @@
>  
>      // Create a LazyScript and initialize closedOverBindings and innerFunctions
>      // with the provided vectors.
>      static LazyScript* Create(JSContext* cx, HandleFunction fun,
> +                              HandleScriptSourceObject sourceObject,

Similarly

@@ +2183,2 @@
>                                const frontend::AtomVector& closedOverBindings,
> +                               Handle<GCVector<JSFunction*, 8>> innerFunctions,

Looks like an inadvertent whitespace change to this line.

@@ +2232,5 @@
>      bool mutedErrors() const {
>          return scriptSource()->mutedErrors();
>      }
>  
> +    void setEnclosingScope(Scope* enclosingScope);

Is my understanding correct that, here, enclosingScope is indeed sometimes nullptr, so Scope* is the right type?

Comment 7

[Jim Blandy :jimb]

Shu explained the Scopes and compartments situation to me on IRC.

At the moment, each Scope does happen to belong to a particular compartment, because Scopes point to Shapes, and each Shape belongs to a particular compartment.

However, it's not essential to Scopes' role that they point to a Shape. They could be redesigned not to, without changing their role much. So to discourage people from relying on the fact that they happen to have a compartment, Scope has no 'compartment' method.

I think the long and short of it is that there should be no assertion in setEnclosingScope.

Comment 8

[Tooru Fujisawa [:arai]]

Thank you for reviewing :D

(In reply to Jim Blandy :jimb from comment #6)
> @@ +2167,5 @@
> >      // Create a LazyScript without initializing the closedOverBindings and the
> >      // innerFunctions. To be GC-safe, the caller must initialize both vectors
> >      // with valid atoms and functions.
> >      static LazyScript* CreateRaw(JSContext* cx, HandleFunction fun,
> > +                                 HandleScriptSourceObject sourceObject,
> 
> Similarly
> 
> @@ +2178,5 @@
> >  
> >      // Create a LazyScript and initialize closedOverBindings and innerFunctions
> >      // with the provided vectors.
> >      static LazyScript* Create(JSContext* cx, HandleFunction fun,
> > +                              HandleScriptSourceObject sourceObject,
> 
> Similarly

Replacing it with ScriptSourceObject& will require extra Rooting inside the method, to avoid rooting hazard.  Is it fine?
or perhaps there is any other way? (like, non-null handle type?)

Comment 9

[Jim Blandy :jimb]

(In reply to Tooru Fujisawa [:arai] from comment #8)
> Replacing it with ScriptSourceObject& will require extra Rooting inside the
> method, to avoid rooting hazard.  Is it fine?
> or perhaps there is any other way? (like, non-null handle type?)

I am very sorry - I misread the LazyScript::CreateRaw and LazyScript::Create declarations. For Handle types, we have no nice way to distinguish between nullable and non-nullable,  so we don't bother trying to make the distinction. Only the LazyScript constructor should be changed, I guess.

Comment 10

[Tooru Fujisawa [:arai]]

(In reply to Jim Blandy :jimb from comment #9)
> (In reply to Tooru Fujisawa [:arai] from comment #8)
> > Replacing it with ScriptSourceObject& will require extra Rooting inside the
> > method, to avoid rooting hazard.  Is it fine?
> > or perhaps there is any other way? (like, non-null handle type?)
> 
> I am very sorry - I misread the LazyScript::CreateRaw and LazyScript::Create
> declarations. For Handle types, we have no nice way to distinguish between
> nullable and non-nullable,  so we don't bother trying to make the
> distinction. Only the LazyScript constructor should be changed, I guess.

okay, thanks!

Comment 11

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6d3ea212415298d10b0ba08a479c7f1a5d02030
Bug 1459127 - Store ScriptSourceObject reference into LazyScript inside LazyScript. r=jimb

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f6d3ea212415