1459220 - Add LazyScript::isEnclosingScriptLazy and replace existing LazyScript::sourceObject callsite which checks if the enclosing script is lazy

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Tooru Fujisawa [:arai]]

this is a preparation for bug 1459127

currently LazyScript.sourceObject_ field is null when the enclosing script is also lazy, and there are several places that checks that field via LazyScript::sourceObject().

I'm about to set LazyScript.sourceObject_ regardless of the enclosing script's laziness, to make debugger faster.
Thus, we cannot use LazyScript.sourceObject_ field for the check.

Then, enclosingScope_ is also null for such case, and we can use that field for the check.

in anyway, it's better adding method that explicitly says it's for checking enclosing script's laziness, instead of LazyScript::sourceObject().

Comment 1

[Tooru Fujisawa [:arai]]

there's LazyScript::hasUncompiledEnclosingScript, which looks like a bit different than this use case.

https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/js/src/vm/JSScript.cpp#4376-4392
> bool
> LazyScript::hasUncompiledEnclosingScript() const
> {
>     // It can happen that we created lazy scripts while compiling an enclosing
>     // script, but we errored out while compiling that script. When we iterate
>     // over lazy script in a compartment, we might see lazy scripts that never
>     // escaped to script and should be ignored.
>     //
>     // If the enclosing scope is a function with a null script or has a script
>     // without code, it was not successfully compiled.


it would be better clarify what the difference is.

Comment 2

[Tooru Fujisawa [:arai]]

Also, it might be better returning reference instead of pointer from sourceObject() methods (JSScript and LazyScript), so that it's clear that the method cannot be used for check.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Add LazyScript::isEnclosingScriptLazy.

Added LazyScript::isEnclosingScriptLazy and replaced the consumer of LazyScript::sourceObject which just checks if the enclosing script is lazy or not.

The content of LazyScript::isEnclosingScriptLazy will be replaced to check enclosingScope_ in bug 1459127.

Comment 4

[Jim Blandy :jimb]

(In reply to Tooru Fujisawa [:arai] from comment #0)
> in anyway, it's better adding method that explicitly says it's for checking
> enclosing script's laziness, instead of LazyScript::sourceObject().

hear, hear

Comment 5

[Jim Blandy :jimb]

(In reply to Tooru Fujisawa [:arai] from comment #2)
> Also, it might be better returning reference instead of pointer from
> sourceObject() methods (JSScript and LazyScript), so that it's clear that
> the method cannot be used for check.

Yes. You may know this, but SpiderMonkey style generally recommends using references for pointers that can never be null.

Comment 6

[Jim Blandy :jimb]

Comment on attachment 8974283 [details] [diff] [review]
Add LazyScript::isEnclosingScriptLazy.

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

Looks good. A few minor changes:

::: js/src/frontend/BytecodeCompiler.cpp
@@ +737,5 @@
>  bool
>  frontend::CompileLazyFunction(JSContext* cx, Handle<LazyScript*> lazy, const char16_t* chars, size_t length)
>  {
>      MOZ_ASSERT(cx->compartment() == lazy->functionNonDelazifying()->compartment());
> +    MOZ_ASSERT(!lazy->isEnclosingScriptLazy());

It might be helpful to put a comment above this:

// We can't be running this script unless we've run its parent.

::: js/src/vm/JSScript.h
@@ +2387,1 @@
>      bool hasUncompiledEnclosingScript() const;

The term 'uncompiled' is unhelpful here, because it's reasonable to consider a lazy script 'uncompiled', but we want to cover only the special case of discarded incomplete scripts. Could we rename this function 'hasUncompletedEnclosingScript'? Can you think of a better term? (This function deserves an ugly name, so ugly is good...)

Comment 7

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c1d3f24941ae0710a55bdcae8ee3d2126ab5b82
Bug 1459220 - Add LazyScript::isEnclosingScriptLazy. r=jimb

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/1c1d3f24941a