Closed Bug 917996 Opened 7 years ago Closed 6 years ago

Add support for LazyScripts in XDRScript.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: nbp, Assigned: nbp)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

As we want to minimize the amount of cache used for the start-up of Gaia application, we do not want to save the bytecode of all functions which are not used during the start-up of an application.

Adding support for LazyScripts implies saving all bindings and source locations, almost as we do for JSScript such as, we can de-lazify them after loading the enclosing source.
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Attached patch /XDR Lazy Scripts (obsolete) — Splinter Review
This patch add the ability to encode/decode LazyScript.
(I forgot to attach it on bugzilla and get it review)
Attachment #8361728 - Flags: review?(luke)
Have you done any measurements comparing this with just plain XDR'ing Gaia apps?  It seems like we should get that working and measured before doing further optimizations, especially since, for the reasons discussed on IRC, this could potentially be worse without chunked script source.
(In reply to Luke Wagner [:luke] from comment #2)
> Have you done any measurements comparing this with just plain XDR'ing Gaia
> apps?  It seems like we should get that working and measured before doing
> further optimizations, especially since, for the reasons discussed on IRC,
> this could potentially be worse without chunked script source.

Doing XDR without encoding LazyScript would make us load more things before starting the execution (XDR is larger than the Source) which is exactly what we are trying to avoid, as the SD Card is the slow point here.

The start-up of applications use only a small subset of the functions, before waiting for any user events.  This means that encoding LazyScripts will make this smaller.

The start-up of gaia applications always used the same functions, as the number of delazified function remained unchanged between multiple runs.  This means that we will reuse the same subset as the serialized one.

Please refer to attachment 805328 [details], on Bug 900784 to get an idea of the order of magnitude.

The goal is to load the source completely, as it is today.  The only different being that we will load it asynchronously instead of waiting for it for starting the execution of the same JavaScript functions as in the previous execution.

For the moment there is no mechanism for storing the XDR buffer next to the cache, nor loading the script source asynchronously.  These should be added.  Testing XDR vs. Source as you suggest is unlikely to be interesting, as the slow point is the SD card and not the parser.

I can try testing the XDR buffer with lazy scripts and with fake sources, but this would mean that the application would not work, as it will likely need the source to respond to user interactions.

Later, we can investigate if loading chunk of the sources is interesting *in addition* to this modification.  But this is another issue.
(In reply to Nicolas B. Pierron [:nbp] from comment #3)
Alright, I can review it then.

> The start-up of gaia applications always used the same functions, as the
> number of delazified function remained unchanged between multiple runs. 
> This means that we will reuse the same subset as the serialized one.

Does this imply that you are planning to XDR, in addition to the top-level script, the small subset of functions that are used during startup?  It seems like this would both save parse time of those functions and also delay the first lazy-function call that blocks on the full source being loaded.
(In reply to Luke Wagner [:luke] from comment #4)
> (In reply to Nicolas B. Pierron [:nbp] from comment #3)
> > The start-up of gaia applications always used the same functions, as the
> > number of delazified function remained unchanged between multiple runs. 
> > This means that we will reuse the same subset as the serialized one.
> 
> Does this imply that you are planning to XDR, in addition to the top-level
> script, the small subset of functions that are used during startup?  It
> seems like this would both save parse time of those functions and also delay
> the first lazy-function call that blocks on the full source being loaded.

This is exactly what I am aiming for.
That sounds pretty nice, then!
Comment on attachment 8361728 [details] [diff] [review]
/XDR Lazy Scripts

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

Overall, this looks good, but a few non-nits so I'd like to see the next version of the patch.

::: js/src/jsfun.cpp
@@ +435,5 @@
> +        if (firstword & IsLazy) {
> +            fun->initLazyScript(lazy);
> +            JS_ASSERT(!lazy->sourceObject());
> +            ScriptSourceObject *sourceObject =
> +                &UncheckedUnwrap(enclosingScript->sourceObject())->as<ScriptSourceObject>();

Rather than copying the body of JSScript::scriptSource, I'd make a new
  ScriptSourceObject *JSScript::sourceObjectUnwrapped() const
that was called from JSScript::scriptSource and here.

::: js/src/jsobj.cpp
@@ -1990,5 @@
>      // Recursively copy dense elements.
>      {
> -        JS::Zone *zone = nullptr;
> -        if (mode == XDR_DECODE)
> -            zone = obj->zone();

I'm confused by this change and the underlying function doesn't appear to be on trunk yet.

::: js/src/jsscript.cpp
@@ +816,1 @@
>                      return false;

You can't 'return false' without reporting an error.  Really, this function shouldn't be called with anything other than the above two cases (the only exception would be an asm.js module, but those shouldn't (yet) be XDR'd).  Thus, it seems preferable to have:
  if (function->isInterpretedLazy()) {
    ...
  } else {
    scope = function->nonLazyScript()->enclosingStaticScope();
  }
(nonLazyScript() already asserts hasScript for you)

@@ -820,3 @@
>              if (!xdr->codeUint32(&funEnclosingScopeIndex))
>                  return false;
> -            Rooted<JSObject*> funEnclosingScope(cx);

This was a good name, so can you rename 'scope' to 'funEnclosingScope'.

@@ +962,5 @@
> +    // If the script has been de-lazified, then encode the delazified variant of
> +    // it, this way we will avoid keeping an unused lazy script around.
> +    if (end < begin) {
> +        if (mode == XDR_ENCODE)
> +            script.set(lazy->maybeScript());

Instead of this runaround (and the ad hoc end == UINT32_MAX limitation), can you have XDRInterpretedFunction treat a (fun->isInterpretedLazy() && fun->lazyScript()->maybeScript() != null) function as a normal non-lazy script and call XDRScript directly?

@@ +985,5 @@
> +            return false;
> +        }
> +
> +        if (mode == XDR_DECODE) {
> +            lazy = LazyScript::Create(cx, fun, packed, begin, end,

LazyScript::Create is a pretty bad API since the LazyScript returned isn't GC-safe: the table_ member is marked but not initialized (its memory is just malloc_'d).  This would be ok, as it is in finishFunctionDefinition, if you initialized it before the first possible GC, but XDRAtom/ADRInterpretedFunction can GC, I believe.  The simplest fix would be, I think, for LazyScript::Create to initialize table_'s elements to benign values.

@@ +3401,5 @@
> +        return NULL;
> +
> +    // We just copy every bits back to make sure we restore them correctly as
> +    // the constructor does not accept all informations.
> +    res->packed_ = packedData;

Can you better factor the code instead so that you don't need to do this?  In particular, it seems like you could have the *other* LazyScript::Create pack up the arguments and call *this* function.

::: js/src/jsscript.h
@@ +1800,5 @@
>          return mallocSizeOf(table_);
>      }
> +
> +    uint64_t packed() const {
> +        return packed_;

How about packedFields()/packedFiles_?  "packed" is a bit abstract
Attachment #8361728 - Flags: review?(luke)
(In reply to Luke Wagner [:luke] from comment #7)
> ::: js/src/jsobj.cpp
> @@ -1990,5 @@
> >      // Recursively copy dense elements.
> >      {
> > -        JS::Zone *zone = nullptr;
> > -        if (mode == XDR_DECODE)
> > -            zone = obj->zone();
> 
> I'm confused by this change and the underlying function doesn't appear to be
> on trunk yet.

This is part of the previous patch, as this is unused code.

> @@ +962,5 @@
> > +    // If the script has been de-lazified, then encode the delazified variant of
> > +    // it, this way we will avoid keeping an unused lazy script around.
> > +    if (end < begin) {
> > +        if (mode == XDR_ENCODE)
> > +            script.set(lazy->maybeScript());
> 
> Instead of this runaround (and the ad hoc end == UINT32_MAX limitation), can
> you have XDRInterpretedFunction treat a (fun->isInterpretedLazy() &&
> fun->lazyScript()->maybeScript() != null) function as a normal non-lazy
> script and call XDRScript directly?

I asked till about this, I added a comment in consequence.
Doing so prevent doing relazification of the function once decoded.  Brian suggested a way to address this issue, so in the mean time I am not going to save the lazy script and we can add this later if this becomes a concern.
Attached patch XDR Lazy Scripts (obsolete) — Splinter Review
Delta:
 - Rename packed to packedFields
 - Rename scope to its original name (funEnclosingScope)
 - Swap Create functions such as we do not unpacked packed data. (update LazyScript constructor accordingly)
 - Add a Create function used by XDRLazyScript such as the table is not left uninitialized.
 - Add scriptSourceUnwrap() as suggested.
 - Do not return false in XDRScript and expect an assertion if an AsmJSModule is given to encode with XDR. (this should later be addressed in Bug 959268)
 - Do not save LazyScript's begin & end locations if the function is deserialized.
Attachment #8361728 - Attachment is obsolete: true
Attachment #8363118 - Flags: review?(luke)
(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> I asked till about this, I added a comment in consequence.
> Doing so prevent doing relazification of the function once decoded.  Brian
> suggested a way to address this issue, so in the mean time I am not going to
> save the lazy script and we can add this later if this becomes a concern.

Note that I still think you should measure the impact. If it causes a lot of functions not to be relazified anymore, we might want to only land this with the ability to relazify without a LazyScript (or, to recreate lazyscripts from jsscripts, rather).
Ah, re:relazification, that is an interesting subtly I missed when reviewing.

So, just to check my understanding of all this: if we have a lazy function and it is delazified, it changes, in place, to a normal non-lazy interpreted function, right?  If so, then the only time we'd have one of these lazy-functions-that-has-a-script is when you have two JSFunctions sharing a LazyScript, and one is delazified.  Am I correct that this is the expected case: the compiler-created JSFunction is always cloned at runtime so the clone will be delazified and the compiler-created function stay lazy, and *that* is why we would want to handle the lazy-but-has-a-script case?
(In reply to Luke Wagner [:luke] from comment #11)
> So, just to check my understanding of all this: if we have a lazy function
> and it is delazified, it changes, in place, to a normal non-lazy interpreted
> function, right?

Correct.

> If so, then the only time we'd have one of these
> lazy-functions-that-has-a-script is when you have two JSFunctions sharing a
> LazyScript, and one is delazified.

Almost correct: we never have lazy-functions-with-script. What we can have is a lazy function that, in itself, isn't special at all. Additionally, we have a non-lazy function that was once (shallow-)cloned from the lazy one. Its script is tightly bound to the original, now-lazy, function and hence can't be re-targeted to the non-lazy function. So we can have lazy-functions-that-are-still-canonical-owners-of-scripts.

> Am I correct that this is the expected
> case: the compiler-created JSFunction is always cloned at runtime so the
> clone will be delazified and the compiler-created function stay lazy, and
> *that* is why we would want to handle the lazy-but-has-a-script case?

Roughly, yes (except for the last multi-part-term, as explained above).

And this makes me realize that there's a slight problem with relazification right now: when relazifying the canonical function, we reset LazyScript::script_ in order to enable freeing the script. However, if we still have clones around, we won't actually collect the script. If the canonical function is later delazified, we end up with two distinct scripts. This isn't a correctness problem, but it means that the overhead of the next delazification is much higher than it needs to be. I think this should be fixable by making LazyScript::script_ a weakptr.
Just to make sure I understand correctly, I will try to re-state what was said above:

  When we execute/compile a JSOP_LAMBDA, and if this is not a singleton (might be executed multiple times) then we clone the JSFunction, and mark its LazyScript as has-been-cloned.  If the cloned JSFunction is later executed, then we set the script on the original LazyScript.

  If this is a singleton, then we just substitute the JSFunction by its non-lazy variant, but we attach the LazyScript to the JSScript, only if there is no innerFunction (as we need them for the scope chain).

The fact that this is not a singleton implies that we could have a LazyScript which corresponds to a lambda within a loop

  function init() {
    for (…; … ; …) {
      function f() { … };
      f();
    }
  }

In which case, it make sense to relazify such decoded lambda after the start-up, as they are less likely to be used.

Thus we should either save the LazyScript stubs or recover them from the source later.  The problem raised here is between the time spent loading extra LazyScript which are not used during the start-up and the complexity of recovering them later.

For now, the easiest approach which seems to be causing less trouble might be to store the LazyScript info complement if there is no innerFunction in the JSScript.

till: Is there a big difference between restoring a lazy-function-which-hold-a-script vs a non-lazy-function-which-hold-a-lazy-script?  If not I would take the approach of restoring the second one, as we can focus on doing the XDR of the complement of info needed to compute the LazyScript.

Note: If we intent to cache the bytecode after the start-up execution, we should disable the relazification until the serialization is completed/aborted.  Otherwise, we might encode a relazified script which was used during the start-up, which is against the logic of doing this bytecode cache.  The goal being to delay the I/O of the source.
(In reply to Nicolas B. Pierron [:nbp] from comment #13)
> Just to make sure I understand correctly, I will try to re-state what was
> said above:

This is still not entirely correct; I'll try to explain inline:

> 
>   When we execute/compile a JSOP_LAMBDA, and if this is not a singleton
> (might be executed multiple times) then we clone the JSFunction, and mark
> its LazyScript as has-been-cloned.

The other way around: if the fun is a singleton, and has never been cloned, we reuse it instead of cloning. At the same time, we mark it as hasBeenCloned, so this only happens once.

> If the cloned JSFunction is later
> executed, then we set the script on the original LazyScript.

This happens in the case where the above didn't apply: we actually clone the function, instead of reusing it. In that case, we need to have the original, canonical function be non-lazy when executing the clone, because parts of the engine use script->function() to access the function, which has to be the canonical one.

> 
>   If this is a singleton, then we just substitute the JSFunction by its
> non-lazy variant, but we attach the LazyScript to the JSScript, only if
> there is no innerFunction (as we need them for the scope chain).

We do this for all functions (that have no inner functions), clones and originals.

> 
> The fact that this is not a singleton implies that we could have a
> LazyScript which corresponds to a lambda within a loop
> 
>   function init() {
>     for (…; … ; …) {
>       function f() { … };
>       f();
>     }
>   }
> 
> In which case, it make sense to relazify such decoded lambda after the
> start-up, as they are less likely to be used.

Yes, and we can currently relazify them, see above.

Note that relazification only really makes sense if *all* functions using a specific script are relazified. We cannot meaningfully know this beforehand, though, so we eagerly relazify, and the script only gets collected if it so happens that all functions using it are relazified.

> 
> Thus we should either save the LazyScript stubs or recover them from the
> source later.  The problem raised here is between the time spent loading
> extra LazyScript which are not used during the start-up and the complexity
> of recovering them later.

Correct. I don't know how much effort restoring the lazyscript is. We should actually do it eagerly, because it lets us skip parsing entirely if a function with the same source location is parsed a second time - anywhere in the runtime.

This is probably very relevant here, as decoding the script is going to be more expensive than cloning an existing script into another compartment.

> 
> For now, the easiest approach which seems to be causing less trouble might
> be to store the LazyScript info complement if there is no innerFunction in
> the JSScript.

I agree.

> 
> till: Is there a big difference between restoring a
> lazy-function-which-hold-a-script vs a
> non-lazy-function-which-hold-a-lazy-script?

Again: there is no such thing as a lazy-function-which-holds-a-script. The canonical function can be lazy while the script is used by non-lazy clones. In that case, it's delazified before the clone is executed or compiled. To (hopefully) make things entirely clear, here's a list of states that might seem possible, with their actual possibility pointed out:

JSFunction-with-JSScript-without-LazyScript:
possible, for non-lazily-parsed functions, and functions that have inner functions

JSFunction-with-JSScript-with-LazyScript:
possible, for lazy-parsed and then delazified functions without inner functions

JSFunction-with-JSScript-and-LazyScript:
not possible, those two are stored in a union

JSFunction-with-LazyScript-without-JSScript:
possible, for lazy-parsed functions that haven't been de-lazified, and relazified original functions

JSFunction-with-LazyScript-with-JSScript:
possible, for re-lazified cloned functions


> If not I would take the
> approach of restoring the second one, as we can focus on doing the XDR of
> the complement of info needed to compute the LazyScript.

As said above, I think a function restored from XDR should always be accompanied by a LazyScript, to enable cloning-instead-of-parsing-or-restoring for all consecutive uses of the same script. Whether the LazyScript is serialized or restored from the JSScript itself is secondary from this point of view.

> 
> Note: If we intent to cache the bytecode after the start-up execution, we
> should disable the relazification until the serialization is
> completed/aborted.  Otherwise, we might encode a relazified script which was
> used during the start-up, which is against the logic of doing this bytecode
> cache.  The goal being to delay the I/O of the source.

Agreed. You could set a flag on the compartment which is only removed after startup is complete. This flag can be queried in JSFunction::trace, along with the other conditions for relazification.
Thanks for the in-depth responses Till.  For one thing, you have convinced me you should review this patch :)  Would you be so kind?

By "lazy-functions-that-has-a-script" I did indeed what you more accurately referred to as "JSFunction-with-LazyScript-with-JSScript".  To test my understanding, though: this won't occur for the simple case of a non-singleton canonical JSFunction that is cloned and run: you said that, in this case, we mutate the canonical function to be non-lazy as well as the clone?

Do you suppose you could file a bug to write a nice beefy comment (probably right before the LazyScript definition) that lays out these possible configurations and the transitions between them?  Or maybe you already wrote this up else-where and you could refer to it from the LazyScript comment?
(In reply to Luke Wagner [:luke] from comment #15)
> Thanks for the in-depth responses Till.  For one thing, you have convinced
> me you should review this patch :)  Would you be so kind?

I *did* expect that doing this work would cause me more work. ;) Sure, I'll do the review.

> 
> By "lazy-functions-that-has-a-script" I did indeed what you more accurately
> referred to as "JSFunction-with-LazyScript-with-JSScript".  To test my
> understanding, though: this won't occur for the simple case of a
> non-singleton canonical JSFunction that is cloned and run: you said that, in
> this case, we mutate the canonical function to be non-lazy as well as the
> clone?

That's correct, yes. Only if the stays around and is not relazified (presumably because it also got JITted) while the canonical function is relazified can this situation occur.

> 
> Do you suppose you could file a bug to write a nice beefy comment (probably
> right before the LazyScript definition) that lays out these possible
> configurations and the transitions between them?  Or maybe you already wrote
> this up else-where and you could refer to it from the LazyScript comment?

That's a good idea. There is a pretty detailed comment on the conditions under which relazification can happen in JSFunction::trace, but documenting the other side of this coin makes sense. Will file.
Comment on attachment 8363118 [details] [diff] [review]
XDR Lazy Scripts

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

Overall, this looks good. Canceling review not so much because I absolutely must see a second version.

Rather, I think this should only land if either you can provide measurements showing that not being able to delazify non-lazy XDR'd functions doesn't cause memory overhead, or you add support for restoring the LazyScript from the JSScript. Or you store the LazyScript along with the JSScript after all. Maybe with the duplicated information stored only once and applied to both resulting objects upon decoding.

::: js/src/jsfun.cpp
@@ +369,5 @@
>  
>      JSContext *cx = xdr->cx();
>      RootedFunction fun(cx);
>      RootedScript script(cx);
> +    LazyScript *lazy = nullptr;

Rooted<LazyScript *>

@@ +389,5 @@
>          if (fun->isStarGenerator())
>              firstword |= IsStarGenerator;
> +
> +        if (fun->isInterpretedLazy() && !fun->lazyScript()->maybeScript()) {
> +            /* Encode a lazy script */

nit: c++-style comment and "." at the end

@@ +393,5 @@
> +            /* Encode a lazy script */
> +            firstword |= IsLazy;
> +            lazy = fun->lazyScript();
> +        } else if (fun->isInterpretedLazy()) {
> +            /* Encode the delazified script */

nit: c++-style comment and "." at the end

@@ +395,5 @@
> +            lazy = fun->lazyScript();
> +        } else if (fun->isInterpretedLazy()) {
> +            /* Encode the delazified script */
> +            /*
> +             * Note: Encoding delazified functions as plain JSScript currently

s/functions/scripts/

@@ +397,5 @@
> +            /* Encode the delazified script */
> +            /*
> +             * Note: Encoding delazified functions as plain JSScript currently
> +             * prevent the relazification of LazyScript. On the other hand, this
> +             * avoid encoding the symbol tables twice, as the lazy script

s/avoid/avoids/

@@ +398,5 @@
> +            /*
> +             * Note: Encoding delazified functions as plain JSScript currently
> +             * prevent the relazification of LazyScript. On the other hand, this
> +             * avoid encoding the symbol tables twice, as the lazy script
> +             * duplicate some of them. In addition, later we might be able to

s/duplicate/duplicates/

@@ +400,5 @@
> +             * prevent the relazification of LazyScript. On the other hand, this
> +             * avoid encoding the symbol tables twice, as the lazy script
> +             * duplicate some of them. In addition, later we might be able to
> +             * restore the LazyScript hunk. (see Bug 886193 comment 6)
> +             */

nit: c++-style comment

@@ +403,5 @@
> +             * restore the LazyScript hunk. (see Bug 886193 comment 6)
> +             */
> +            script = fun->lazyScript()->maybeScript();
> +        } else {
> +            /* Encode the script */

nit: c++-style comment and "." at the end

::: js/src/jsscript.cpp
@@ +826,5 @@
>                      funEnclosingScope = script->objects()->vector[funEnclosingScopeIndex];
>                  }
>              }
>  
> +            /* Code nested function and script */

nit: C++-style comment, and "." at the end

@@ +912,5 @@
> +    JSContext *cx = xdr->cx();
> +    LazyScript *lazy = *lazyp;
> +
> +    // Code the location of the script in bytes, if "end" is lower than "begin",
> +    // then we will code/decode the delazified script instead of the lazy one.

I don't understand this comment. Can you explain what you mean, and adapt the comment?

@@ +3327,5 @@
> +    p.usesArgumentsAndApply_ = false;
> +    p.hasBeenCloned_ = false;
> +    p.treatAsRunOnce_ = false;
> +
> +    LazyScript *res = LazyScript::Create(cx, fun, packedFields, begin, end, lineno, column);

I think this needs to be rooted

@@ +3336,5 @@
> +/* static */ LazyScript *
> +LazyScript::Create(ExclusiveContext *cx, HandleFunction fun,
> +                   uint64_t packedFields, uint32_t begin, uint32_t end,
> +                   uint32_t lineno, uint32_t column,
> +                   HandleAtom dummyAtom, HandleFunction dummyFun)

Why do you need to pass in the dummyAtom and dummyFun? Can't you create them internally? If not, please add a comment to the declaration explaining why they have to be parameters.

@@ +3338,5 @@
> +                   uint64_t packedFields, uint32_t begin, uint32_t end,
> +                   uint32_t lineno, uint32_t column,
> +                   HandleAtom dummyAtom, HandleFunction dummyFun)
> +{
> +    LazyScript *res = LazyScript::Create(cx, fun, packedFields, begin, end, lineno, column);

I think this needs to be rooted

@@ +3377,5 @@
>          if (!table)
>              return nullptr;
>      }
>  
>      LazyScript *res = js_NewGCLazyScript(cx);

hmm, but then again, this isn't rooted, so maybe the others don't need to be, either. Please verify

::: js/src/jsscript.h
@@ +535,5 @@
>  
> +template<XDRMode mode>
> +bool
> +XDRLazyScript(XDRState<mode> *xdr, HandleObject enclosingScope, HandleScript enclosingScript,
> +              HandleFunction fun, LazyScript **lazy, MutableHandleScript script);

I think lazy should be MutableHandle<LazyScript *>
Also, it should be "lazyp", to match the implementation (and the usage)

@@ +1624,5 @@
> +        uint32_t hasDebuggerStatement_ : 1;
> +        uint32_t directlyInsideEval_:1;
> +        uint32_t usesArgumentsAndApply_:1;
> +        uint32_t hasBeenCloned_:1;
> +        uint32_t treatAsRunOnce_:1;

all fields in PackedView should lose the "_" suffix

@@ +1649,2 @@
>    public:
> +    // Warning: After calling this fucntion and checking the returned value, the

s/fucntion/function/

@@ +1649,3 @@
>    public:
> +    // Warning: After calling this fucntion and checking the returned value, the
> +    // freeVariables and innerFunctions have to be initialized.

"must be initialized by the callee", maybe? Otherwise, it sounds like they should be assumed to be initialized.
Attachment #8363118 - Flags: review?(luke)
Random thought: it's great to see you guys putting in some serious effort to reduce SpiderMonkey's memory consumption.
Depends on: 963589
I have no way to recover all the freeVariable, for the moment Bug 965722 is blocking Bug 963589, and there is no work around to recover the name.  So I suggest that we save the free variables as part of the XDRScript and reconstruct a LazyScript when we decode the function.

This is not ideal, but I do not want to be blocked on Bug 965722 for the moment.
I agree, we should do that.
(In reply to Till Schneidereit [:till] from comment #17)
> @@ +3338,5 @@
> > +                   uint64_t packedFields, uint32_t begin, uint32_t end,
> > +                   uint32_t lineno, uint32_t column,
> > +                   HandleAtom dummyAtom, HandleFunction dummyFun)
> > +{
> > +    LazyScript *res = LazyScript::Create(cx, fun, packedFields, begin, end, lineno, column);
> 
> I think this needs to be rooted
> 
> @@ +3377,5 @@
> >          if (!table)
> >              return nullptr;
> >      }
> >  
> >      LazyScript *res = js_NewGCLazyScript(cx);
> 
> hmm, but then again, this isn't rooted, so maybe the others don't need to
> be, either. Please verify

From what I understand, we do not need to Root any pointer if it is not going to be mutated by a GC.
This means that the return value of an allocation does not need to be rooted until the time when we are doing another allocation.

These LazyScript::Create are only doing one allocation, and none after the Create call.  This means that we do not need to root the LazyScript* before returning it as it cannot be moved by the GC.

> @@ +1649,3 @@
> >    public:
> > +    // Warning: After calling this fucntion and checking the returned value, the
> > +    // freeVariables and innerFunctions have to be initialized.
> 
> "must be initialized by the callee", maybe? Otherwise, it sounds like they
> should be assumed to be initialized.

As I removed the dummies from the last Create, I renamed the 2 previous one CreateRaw, and added the following comment above the 2 CreateRaw functions:

    // Create a LazyScript without initializing the freeVariables and the
    // innerFunctions. To be GC-safe, the caller must initialize both vectors
    // with valid atoms and functions.

This way we have CreateRaw, which is not complete nor GC-safe, and one Create function, which fills the freeVariables & innerfFunctions with dummies.
Attached patch XDR Lazy ScriptsSplinter Review
Delta:
 - Remove handling of JSFunction-with-Lazy-with-Script and asserts that it cannot be seen, as this is a runtime-only case.
 - Rename Create to CreateRaw when the vectors are not initialized.
 - Reset runtime flags in CreateRaw (hasBeenCloned & treatAsRunOnce)
 - Save Scripts' LazyScripts. (also see attached test cases)
   - We need to save freeVariables, because we have no good solution for the moment (see Bug 963589)
   - We need to save the LazyScript flags, as they do not match the JSScript flags. (see Bug 900789, assertEqBytecode check that the encoded bytecode is the same after being loaded from the source or after being decoded from the cache)
Attachment #8363118 - Attachment is obsolete: true
Attachment #8377165 - Flags: review?(till)
This patch is made on top of Bug 900789 (which now contains more test cases in additional files).

These additional tests are checking that we can relazify functions.
Attachment #8377189 - Flags: review?(till)
Comment on attachment 8377165 [details] [diff] [review]
XDR Lazy Scripts

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

Looks good! I've commented quite a bit, but most of the feedback is focussed on naming and comments, so r=me with comments addressed.

::: js/src/jsscript.cpp
@@ +413,5 @@
>  
>  template bool
>  js::XDRScriptConst(XDRState<XDR_DECODE> *, MutableHandleValue);
>  
> +// Code the missing part needed to re-create a LazyScript from a JSScript.

Please expand this comment to explain why this is needed and what info is XDR'd

@@ +416,5 @@
>  
> +// Code the missing part needed to re-create a LazyScript from a JSScript.
> +template<XDRMode mode>
> +static bool
> +XDRLazyPart(XDRState<mode> *xdr, HandleFunction fun, HandleScript script,

How about XDRRelazificationInfo? It's hard to understand what this function does, as LazyPart doesn't really describe its functionality. RelazificationInfo seems like a good, if not perfect, fit

@@ +428,5 @@
> +    uint32_t end = script->sourceEnd();
> +    uint32_t lineno = script->lineno();
> +    uint32_t column = script->column();
> +    uint64_t packedFields;
> +    {

What's the reason for putting this code and the free vars coding below in blocks? They don't seem to step on each other's toes at all. If you choose to keep the blocks, at least move the variables only used in a block into that block. (But I'd remove them, personally.)

@@ +435,5 @@
> +
> +            // Assert equivalence relations of runtime flags.
> +            JS_ASSERT(script->treatAsRunOnce() == lazy->treatAsRunOnce());
> +            // This assertion does not hold for the moment.
> +            // JS_ASSERT(script->hasBeenCloned() == lazy->hasBeenCloned());

It doesn't? It either should, or the assert and comment be removed. Or, if that involves too much work, at least explain why it's ok to have it not hold for now, create a bug for fixing it, and mention that in the comment.

@@ +466,5 @@
> +
> +            if (mode == XDR_DECODE)
> +                freeVariables[i] = atom;
> +        }
> +    }

I'm not too wild about the duplication between XDRLazyScript and XDRLazyPart. Can you at least move the free var coding into a helper function?

@@ +468,5 @@
> +                freeVariables[i] = atom;
> +        }
> +    }
> +
> +    // At the moment, we do not relazify functions which have inner functions.

s/At the moment//
If we change that, this assert will trigger and the comment will be changed, so no need to mention that we might do something else here at some point. Also, can you move this to the function's top and make it an ASSERT_IF, too?

@@ +884,5 @@
>  
>            case CK_JSFunction: {
>              /* Code the nested function's enclosing scope. */
>              uint32_t funEnclosingScopeIndex = 0;
> +            RootedObject funEnclosingScope(cx);

Please move this into the if block - no need to create the root when decoding

@@ +973,5 @@
>  
> +    if (scriptBits & (1 << HasLazyScript)) {
> +        Rooted<LazyScript *> lazy(cx);
> +        if (mode == XDR_ENCODE) {
> +            lazy = script->maybeLazyScript();

Can you MOZ_ASSERT(lazy)? (Or remove the braces.)

@@ +979,5 @@
> +
> +        if (!XDRLazyPart(xdr, fun, script, &lazy))
> +            return false;
> +
> +        if (mode == XDR_DECODE) {

Nit: no braces needed

@@ +1015,5 @@
> +    JSContext *cx = xdr->cx();
> +
> +    uint32_t begin;
> +    uint32_t end;
> +    {

Here, too, please either remove the blocks, or move the variables only used within one block into that block (in this case: lineno, column, packedFields)

@@ +1017,5 @@
> +    uint32_t begin;
> +    uint32_t end;
> +    {
> +        if (mode == XDR_ENCODE) {
> +            JS_ASSERT(!lazy->maybeScript());

Can you move this to the function's top and turn it into a MOZ_ASSERT_IF?

::: js/src/jsscript.h
@@ +1720,5 @@
> +                                 JSVersion version, uint32_t begin, uint32_t end,
> +                                 uint32_t lineno, uint32_t column);
> +
> +    // Create a LazyScript and initialize the freeVariables and the
> +    // innerFunctions with dummies which are values which cannot be produced in

maybe "[..] with dummy values to be replaced in a later initialization phase"?
Attachment #8377165 - Flags: review?(till) → review+
Comment on attachment 8377189 [details] [diff] [review]
Test relazification.

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

r=me with feedback addressed

::: js/src/builtin/TestingFunctions.cpp
@@ +356,5 @@
> +    if (argc != 1) {
> +        JS_ReportError(cx, "the function takes exactly one argument");
> +        return false;
> +    }
> +    if (!args[0].isObject() || !args[0].toObject().is<JSFunction>()) {

Either just assert that the argument is a function, or report an error. If asked "is cheese a lazy function", you wouldn't just say "no" either. At least not if you want to be helpful. ;)
(I would just assert, this is a testing function, after all.)

@@ +373,5 @@
> +        JS_ReportError(cx, "the function takes exactly one argument");
> +        return false;
> +    }
> +    if (!args[0].isObject() ||
> +        !args[0].toObject().is<JSFunction>())

Same comment as above

@@ +1630,5 @@
>  "  Returns whether the given value is a nested function in an asm.js module that has been\n"
>  "  both compile- and link-time validated."),
>  
> +    JS_FN_HELP("isLazyFunction", IsLazyFunction, 1, 0,
> +"isLazyFunction(obj)",

s/obj/fun/ (and see comment above)

@@ +1631,5 @@
>  "  both compile- and link-time validated."),
>  
> +    JS_FN_HELP("isLazyFunction", IsLazyFunction, 1, 0,
> +"isLazyFunction(obj)",
> +"  If true, obj is a lazy JSFunction."),

"True if fun is a lazy JSFunction"

@@ +1635,5 @@
> +"  If true, obj is a lazy JSFunction."),
> +
> +    JS_FN_HELP("isRelazifiableFunction", IsRelazifiableFunction, 1, 0,
> +"isRelazifiableFunction(obj)",
> +"  If true, obj is a JSFunction with a relazifiable JSScript."),

Same changes here as above

::: js/src/jit-test/lib/bytecode-cache.js
@@ +6,5 @@
>      lineNumber: { value: 0 }
>    });
>    code = code instanceof Object ? code : cacheEntry(code);
>  
>    // We create a new global

"We create a new global ..."

@@ +10,5 @@
>    // We create a new global
>    if (!("global" in ctx))
>      ctx.global = newGlobal();
>  
> +  // And set compileAndGo by default to emulate the top-level.

"... and by default enable compileAndGo."

@@ +14,5 @@
> +  // And set compileAndGo by default to emulate the top-level.
> +  if (!("compileAndGo" in ctx))
> +    ctx.compileAndGo = true;
> +
> +  // Fetch the verification function.

What "verification function"? Explain what it is supposed to verify.

@@ +17,5 @@
> +
> +  // Fetch the verification function.
> +  var checkAfter = function(ctx) {};
> +  if ("checkAfter" in ctx)
> +    checkAfter = ctx.checkAfter;

var checkAfter = ctx.checkAfter || function(cxt) {};

::: js/src/jit-test/tests/xdr/lazy.js
@@ +1,3 @@
>  load(libdir + 'bytecode-cache.js');
>  var test = "";
> +var checkAfter = function (ctx) { };

No need to add a default here, with the suggested change in bytecode_cache.js

@@ +101,5 @@
>    return f.toSource() + "; f();";
>  })();
>  evalWithCache(test, { assertEqBytecode: true });
> +
> +// Ensure that decoded functions can be relazified

Nit: missing "." at the end

@@ +104,5 @@
> +
> +// Ensure that decoded functions can be relazified
> +test = "function f() { }; f();"
> +  + "assertEq(isLazyFunction(f), false);"
> +  + "var expect = isRelazifiableFunction(f);";

Nit: "+"-es at the end of the previous line. At the very least, left-align them with "function

@@ +106,5 @@
> +test = "function f() { }; f();"
> +  + "assertEq(isLazyFunction(f), false);"
> +  + "var expect = isRelazifiableFunction(f);";
> +checkAfter = function (ctx) {
> +  gc(ctx.global.f); // relazify f, when possible.

s/when/if/

@@ +113,5 @@
> +evalWithCache(test, {
> +  assertEqBytecode: true,  // Check that we re-encode the same thing.
> +  assertEqResult: true,    // The function should remain relazifiable, if it was
> +                           // during the first run.
> +  checkAfter: checkAfter   // Check that the function can be relazified if possible.

"Check that relazifying the restored function works if the original was relazifiable."

@@ +116,5 @@
> +                           // during the first run.
> +  checkAfter: checkAfter   // Check that the function can be relazified if possible.
> +});
> +
> +// Ensure that decoded functions can be relazified, even if it has free

"they have free"

@@ +120,5 @@
> +// Ensure that decoded functions can be relazified, even if it has free
> +// variables.
> +test = "function f() { return isRelazifiableFunction(f) }; var expect = f();"
> +  + "assertEq(isLazyFunction(f), false);"
> +  + "expect";

Same nit as above

@@ +122,5 @@
> +test = "function f() { return isRelazifiableFunction(f) }; var expect = f();"
> +  + "assertEq(isLazyFunction(f), false);"
> +  + "expect";
> +checkAfter = function (ctx) {
> +  gc(ctx.global.f); // relazify f, when possible.

s/when/if/

@@ +129,5 @@
> +evalWithCache(test, {
> +  assertEqBytecode: true,  // Check that we re-encode the same thing.
> +  assertEqResult: true,    // The function should remain relazifiable, if it was
> +                           // during the first run.
> +  checkAfter: checkAfter   // Check that the function can be relazified if possible.

"Check that relazifying the restored function works if the original was relazifiable."
Attachment #8377189 - Flags: review?(till) → review+
(In reply to Till Schneidereit [:till] from comment #24)
> @@ +428,5 @@
> > +    uint32_t end = script->sourceEnd();
> > +    uint32_t lineno = script->lineno();
> > +    uint32_t column = script->column();
> > +    uint64_t packedFields;
> > +    {
> 
> What's the reason for putting this code and the free vars coding below in
> blocks? They don't seem to step on each other's toes at all. If you choose
> to keep the blocks, at least move the variables only used in a block into
> that block. (But I'd remove them, personally.)

The reason is simple, XDR functions can either encode or decode.  When I started working on XDR functions, I noticed that there is a symmetry and that every thing which is coded is   "read & copied & written",  this block syntax is just here to highlight what I found to be a good idiom for writing XDR functions, and ensure that the variables which are still live after the block are initialized independently of the XDR-coding-mode.
Adding a block around the XDR_ENCODE & code & XDR_DECODE, is just a way to highlight this symmetry.
Comment on attachment 8377165 [details] [diff] [review]
XDR Lazy Scripts

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

::: js/src/jsscript.cpp
@@ +920,2 @@
>              RootedObject tmp(cx, *objp);
>              if (!XDRInterpretedFunction(xdr, funEnclosingScope, script, &tmp))

We need to keep funEnclosingScope out-side of the if-block, as it is used by this function.

@@ +973,5 @@
>  
> +    if (scriptBits & (1 << HasLazyScript)) {
> +        Rooted<LazyScript *> lazy(cx);
> +        if (mode == XDR_ENCODE) {
> +            lazy = script->maybeLazyScript();

No need for a MOZ_ASSERT(lazy), as XDRRelazificationInfo is reading elements out of the lazy, so it will SEGV if it is null.  So, I only removed the braces.
Comment on attachment 8377189 [details] [diff] [review]
Test relazification.

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

::: js/src/builtin/TestingFunctions.cpp
@@ +356,5 @@
> +    if (argc != 1) {
> +        JS_ReportError(cx, "the function takes exactly one argument");
> +        return false;
> +    }
> +    if (!args[0].isObject() || !args[0].toObject().is<JSFunction>()) {

As these are used by fuzzers, we should guard instead of asserting, but your point still make sense ;)
http://hg.mozilla.org/integration/mozilla-inbound/rev/ef88599e0dbf

Mark as [leave-open] for landing the test case as soon as all blockers of Bug 900789 are fixed.
Whiteboard: [leave-open]
http://hg.mozilla.org/integration/mozilla-inbound/rev/112accda13eb (tests)
Flags: in-testsuite+
Whiteboard: [leave-open]
https://hg.mozilla.org/mozilla-central/rev/112accda13eb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment on attachment 8377165 [details] [diff] [review]
XDR Lazy Scripts

>From 3b8d6ccd621e6744691a0f56789e210cd30f7b4c Mon Sep 17 00:00:00 2001
>From: "Nicolas B. Pierron" <nicolas.b.pierron@mozilla.com>
>Date: Mon, 9 Dec 2013 08:29:06 -0800
>Subject: [PATCH 01/11] Bug 917996 - XDR Lazy Scripts. r=
>

>+
>+    LazyScript *res = LazyScript::CreateRaw(cx, fun, packedFields, begin, end, lineno, column);
>+    JS_ASSERT(res->version() == version);
>+    return res;
>+}


I'm seeing OOM crashes here, shouldn't this be something like this?

> JS_ASSERT_IF(res, res->version() == version);
Attachment #8377165 - Flags: feedback?(nicolas.b.pierron)
(In reply to Christian Holler (:decoder) from comment #33)
> I'm seeing OOM crashes here, shouldn't this be something like this?
> 
> > JS_ASSERT_IF(res, res->version() == version);

Yes, this should, happy to know that this code path is tested by fuzzers :)
Attachment #8377165 - Flags: feedback?(nicolas.b.pierron)
You need to log in before you can comment on or make changes to this bug.