886193 - Re-lazify non-JITted JSFunctions on GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

What the title says. More specifically, functions get relazified if they:
- started out lazy
- contain the required information for re-lazification
- and don't contain any jit code.

The last bit is true if a function simply hasn't ever gotten hot enough, or if a previous gc has discarded their jit code. In that case, however, the second condition most likely doesn't hold for any functions except self-hosted ones, anymore: to re-lazify, we have to restore the lazy-script info, so we store it in the JSScript to have it available. However, keeping it around for a long time would be wasteful, so it gets discarded on the first GC that doesn't cause re-lazification.

Results are very promising thus far: opening gmail to a folder view and then minizing memory usage via about:memory multiple time, I get a js-main-runtime reduction from ~87Mb to ~77Mb, with more than 3500 scripts re-lazified.

I didn't do any perf benchmarking yet, so I can't promise that this isn't extremely bad for performance, somehow, but I doubt that there are any fundamental issues there.

Also, I suspect that more changes like the one in IonCaches.cpp will be required for this to not crash anywhere. At least for short browsing sessions it doesn't do that for me, however.

Comment 1

[Till Schneidereit [:till]]

Attachment: wip

Comment 2

[Till Schneidereit [:till]]

Attachment: wip 2

Slightly changed to, technically, also work on chrome scripts. However, this doesn't change a thing right now, as lazy scripts aren't actually enabled for chrome scripts; what with the missing source and all ...

We might want to look into enabling that, however: while the percentage of scripts that's actually executed is much higher for chrome scripts than for web ones, many of them might be executed only once or a few times.

First round of try-servering at https://tbpl.mozilla.org/?tree=Try&rev=de87cffc3342

Comment 3

[Jan de Mooij [:jandem]]

What happens to JSScript::types? Is it discarded when we lazify?

Comment 4

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> What happens to JSScript::types? Is it discarded when we lazify?

We don't create script->types these days until the script is baseline compiled, so iiuc none of the relazified scripts will have any type information at all.

Comment 5

[Till Schneidereit [:till]]

(In reply to Brian Hackett (:bhackett) from comment #4)
> (In reply to Jan de Mooij [:jandem] from comment #3)
> > What happens to JSScript::types? Is it discarded when we lazify?
> 
> We don't create script->types these days until the script is baseline
> compiled, so iiuc none of the relazified scripts will have any type
> information at all.

I talked about this with jandem. The way I have it now, self-hosted functions can get re-lazified even after getting jit-compiled. That happens if a gc discards the jit code and the function doesn't get jitted again until the next gc.

For lazily-parsed functions, the LazyScript is thrown away on the first gc that doesn't re-lazify them, so this isn't possible. It might be worthwhile to check if it happens with any frequency that a function gets hot during initial setup of a page and is then cold ever after. If that turns out to be true, we could re-evaluate this.

Comment 6

[Brian Hackett [Laid off!]]

Even if the LazyScript is thrown away it could be reconstructed from the information on the JSScript itself.  The only tricky bit would be with the free variables in the lazy script, and (a) they could be recovered from the bytecode with a bit of complexity, and (b) they are only relevant when compiling an outer lazy function in which the script is nested, and aren't needed at all if e.g. we only relazify functions with no inner functions (not sure how much that would impact the memory improvements).

Comment 7

[Till Schneidereit [:till]]

Ah, that makes sense, nice.

I disabled relazification for functions containing inner functions anyway, so that's not an issue. We should, however, at least check how much of a difference that makes. Might be worthwile to relazify those, too, but only as long as we have the LazyScript around.

Comment 8

[Florian Bender]

(In reply to Till Schneidereit [:till] from comment #7)
> I disabled relazification for functions containing inner functions anyway,
> so that's not an issue. We should, however, at least check how much of a
> difference that makes. Might be worthwile to relazify those, too, but only
> as long as we have the LazyScript around.

How would this effect IIFEs? Do they get special treatment?

Comment 9

[Till Schneidereit [:till]]

> How would this effect IIFEs? Do they get special treatment?

That's one of the things I have to think through before taking any steps to enable this for heavy functions.

Comment 10

[[:fabrice] Fabrice Desré]

Till, do you mind rebasing this patch? We are targetting low memory devices where this could help a lot from what you mention in comment #0. thanks!

Comment 11

[Till Schneidereit [:till]]

I just talked about that with jandem yesterday. Will rebase and attach the patch later today.

Comment 12

[Till Schneidereit [:till]]

Attachment: wip 3

Rebased and somewhat improved. It fails some tests, still, with at least two different exceptions being thrown:

jit-test/tests/gc/incremental-01.js fails with
Assertion failure: MapTypeToTraceKind<T>::kind == GetGCThingTraceKind(thing), at /Users/till/mozilla/mozilla-inbound/js/src/gc/Marking.cpp:149

jit-test/tests/ion/bug756781.js fails with
Assertion failure: [barrier verifier] Unmarked edge: realScript, at /Users/till/mozilla/mozilla-inbound/js/src/gc/Verifier.cpp:580


Clearly, I have no idea what I'm doing wrt the marking stuff. :(

Comment 13

[Jan de Mooij [:jandem]]

I looked at the second failure. I think the problem is that relazifying during marking interacts badly with the pre-barrier verifier.

The verifier traces the heap to collect all edges when an incremental GC starts. Then at the end of the incremental GC, it asserts that for all these edges, the GCThing was marked (or allocated after the incremental GC started).

Now, in this case, StartVerifyPreBarriers traces the heap to collect edges and at this point we relazify the script. After this is done, LazyScript::script_ is never marked; that's the whole point of relazifying :) but it makes the barrier verifier unhappy.

Maybe we could do the unlazifying as a separate phase, like Zone::discardJitCode? I'm not a GC person, so Terrence or Bill please correct me if I'm wrong.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm having a little trouble understanding the patch since I don't know much about lazy scripts. However, here's some information that might help.

I can see one possible issue with this patch. Let's say we start an incremental GC. Let S be a non-lazy script. After the first GC slice, we somehow obtain a pointer to the script and write it to some location that doesn't need a barrier, like the stack. Now we trace S and relazify it. From Jan's description, it sounds like we won't mark S at that time. That's a problem if we later access S through the stack location.

I'm a little unsure if this is really a problem for two reasons:
1. Maybe you can't obtain a script from a function in this way? I don't know.
2. It looks like the patch does mark the script that's being overwritten in some cases via resetScript. It looks like that only happens sometimes though?

Anyway, if it's really a problem, then I can see two ways forward:
1. Do what Jan is suggesting and do the relazification outside of GC. This would require a bit of work, since we'd want to traverse the heap incrementally to avoid jank. There's no mechanism to do that right now, although we probably will need one for compacting GC whenever that happens.
2. Do the relazification inside the GC, but continue marking the non-lazy script. That would mean that it couldn't be thrown away until the next GC. Also, we'd need a check to ensure that you only relazify during actual GCs and not during TraceRuntime.

Option 2 is a little scary because we normally avoid changing the heap during GC. But I can't think of any problems with it.

Also, here's a slightly longer explanation of what's going on with the pre-barrier verifier. The pre-barrier verifier runs independent of GC. In fact, a GC will cause us to prematurely end verification. The verifier just uses TraceRuntime, which traces the heap like a GC but doesn't collect anything. However, it looks like we still do relazification during TraceRuntime. So the verifier is causing the relazification code to be run during verification, and it's observing that we're overwriting pointers without a barrier. That's why it's complaining.

By this way, this is a nice patch. It's really lovely how simple it is.

Comment 15

[Terrence Cole [:terrence]]

Comment on attachment 8350064 [details] [diff] [review]
wip 3

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

Lets take a look at the first assertion. Every arena's header has a word which tells what kind of things are in that arena -- the AllocKind -- e.g. FINALIZE_OBJECT8, FINALIZE_STRING, FINALIZE_SHORT_STRING, etc. However, multiple AllocKinds are generally marked in the same way. E.g. FINALIZE_OBJECT2 can be marked with the same code as FINALIZE_OBJECT16_BACKGROUND, as FINALIZE_OBJECT4_BACKGROUND; FINALIZE_STRING and FINALIZE_SHORT_STRING; etc. Because this is ancient C code, rather than using types, this relationship is captured by the enum JSGCTraceKind: the first is JSTRACE_OBJECT, the second by JSTRACE_STRING, etc. For the most part, we've already moved SpiderMonkey to use C++ type dispatch to select the right routine based on the pointer type and don't generally use JSGCTraceKind anymore. 

However, you've probably noticed that our Marking functions still embed the trace kind in the names: Mark/Object/(JSObject*), Mark/String/(JSString*), Mark/Object/(JSFunction*), etc. Why? Because history! It's another C-ism: SpiderMonkey only exposes a bodyless JSString, JSScript, and JSObject to the browser. They have no common base class. Thus, whenever the browser wants to track a generic Cell*, it instead uses a void* plus a JSGCTraceKind (it's like a type!). Moreover, up until this year the trace interfaces we've exposed in jsapi.h were all void* + JSGCTraceKind. We've moving in the right direction, but there are still an unfortunate number of untyped void* floating about in the system. The use of the trace kind in the name is not about distrusting C++'s typed dispatch, it's in the vain hope that people will try to understand the above and make double or triple sure that they've lined the C++ type up with the JSGCTraceKind before marking.

And now we finally have enough background to look at the actual assertion. The left side is MapTypeToTraceKind: a static map between C++ types to their attendant JSGCTraceKind -- JSObject* -> JSTRACE_OBJECT, JSString* -> JSTRACE_STRING, etc. The right side maps between the arena's AllocKind and the JSGCTraceKind -- FINALIZE_OBJECT0 -> JSTRACE_OBJECT, FINALIZE_STRING -> JSTRACE_STRING, FINALIZE_SCRIPT -> JSTRACE_SCRIPT, etc. It should be really easy at this point to take a look at both sides and figure out what each should be here and ensure through inspection that the right thing is happening.

Also, because this is really annoying: a cheat sheet.
  JSGCTraceKind is in jspubtd.h
  AllocKind is in gc/Heap.h
  MapTypeToTraceKind is in jsgc.h
  GetJSGCTraceKind is in jsgcinlines.h

::: js/src/jsfun.cpp
@@ +527,5 @@
>          // yet at some points when parsing, and can be lazy with no lazy script
> +        // for self-hosted code.
> +        if (hasScript() && u.i.s.script_) {
> +            if (nonLazyScript()->isRelazifiable() && !compartment()->isSelfHosting)
> +                relazify(trc);

I think it is fine to relazify during marking; you simply need to maintain alive-at-the-beginning more aggressively. With the code structured like this, if any of the conditions in relazify fail, then script_ will not get marked. I think the second assertion failure implies that in some cases |isRelazifyable() && !comp->isSelfHosting()| but that |!maybeLazyScript() || isSelfHostedBuiltin() || lazy->maybeScript() != nonLazyScript()| at the same time. I would plaster this with as many assertions you can think of to solidify the relationships and find out which of the 32 cases above you missed. I would also restructure the code to make it abundantly clear that script_ always gets marked when present. Something like:

if (hasScript() && u.i.s.script_) {
    MarkScriptUnbarriered(trc, &u.i.s.script_, "script");
    if (nonLazyScript()->isRelazifiable() && !compartment()->isSelfHosting)
        relazify(trc);
}

Then in resetScript I'd assert IsScriptMarked(script_) instead of marking at that time. In addition to the assertions, this should keep any other failures from affecting GC correctness.

You could also consider not relazify if !IS_GC_MARKING_TRACER(trc), e.g. only during actual GC's and not during verification, heap dumping, etc. I'm not sure though: it's nice to have the verifiers seeing exactly what gets seen during a normal GC. I would avoid this if at all possible.

Comment 16

[Till Schneidereit [:till]]

Attachment: wip 4

This finally runs through all js- and jit-tests. Try-servering here:
https://tbpl.mozilla.org/?tree=Try

Comment 17

[Till Schneidereit [:till]]

So, this causes lots of crashes. Most or all of them are caused by using JSFunction's nonLazyScript() after the script has been relazified. Unfortunately, we have lots of places where we make sure a function has a script at some point and then assume that this will always hold. With relazification, it's only guaranteed to hold until the next gc.

I see three ways forward here:

1. audit every single use of nonLazyScript manually and replace the unsafe ones with maybeNonLazyScript and delazification as required. This might not work in all cases, as the place where we're accessing the script might be one where we can't gc (but a gc can happen between the last check for script-existence and here).

2. change usage of the JSScript::isRunning flag to some kind of counter (along the lines of `unsigned relazificationGuard`) that is increased each time we check for script-existence and later rely on it after a gc might happen. This could probably be done with a RAII helper in most cases.

3. revert to only relazifying in compartments that have no script activity. I really don't want to go this route because I think that it has a fairly high risk of leaving lots of relazification opportunities on the table. If the other options fail, it should be somewhat straight-forward to do, though.

Jandem, we've been talking about the relazification-guarding mechanism before. Do you have thoughts on how to proceed here?

Comment 18

[Jan de Mooij [:jandem]]

(In reply to Till Schneidereit [:till] from comment #17)
> Jandem, we've been talking about the relazification-guarding mechanism
> before. Do you have thoughts on how to proceed here?

I like the RAII class. It would be really neat if nonLazyScript then could assert that either:

(1) The counter > 0 (there's a guard preventing this script from relazification).
(2) AutoSuppressGC is active (it's okay to call nonLazyScript during JIT compilation etc).

This avoids turning it into a whack-a-mole game: many calls *can* GC, but rarely do so. With strong asserts in place you can be reasonably sure you got most cases if you have a green Try run.

It would also be nice to run jit-tests/jstests with gczeal(2) to trigger GCs more aggressively.

Comment 19

[Jan de Mooij [:jandem]]

Oh and many nonLazyScript callers should just call getOrCreateScript instead. See for instance JS_GetFunctionScript.

There's also places like FunctionToString where we call getOrCreateScript followed by nonLazyScript, we could just use the result of getOrCreateScript instead.

Hopefully you can get rid of most nonLazyScript calls this way.

Comment 20

[Jan de Mooij [:jandem]]

Till: I'll write a patch to fix most of these calls, I should have something in a few hours/days.

Comment 21

[Jan de Mooij [:jandem]]

Attachment: Minor fix

Clearing the isRunning flag when popping a frame is invalid because a script can be on the stack multiple times. Here's a small, untested patch fixing that: for inline frames it sets a bit on the frame, else it saves the previous value.

Comment 22

[Till Schneidereit [:till]]

(In reply to Jan de Mooij [:jandem] from comment #21)
> Clearing the isRunning flag when popping a frame is invalid because a script
> can be on the stack multiple times. Here's a small, untested patch fixing
> that: for inline frames it sets a bit on the frame, else it saves the
> previous value.

I forgot to mention that I wanted to do 2) from comment 17 in any case, to get this exact issue fixed. Your solution works just as well for that, so I don't much care which we use, if your changes obviate the other reasons for doing 2).

Comment 23

[Luke Wagner [:luke]]

It sounds like you guys are happy with (2) but I just wanted to point out that, in favor of (3), most GCs run without anything on the stack, so I wouldn't expect it to have any effect on normal browser memory usage.  In contrast, it might actually be useful to say that any in-use compartment (as judged by JSCompartment::hasBeenEntered()) isn't considered for re-lazification as it avoids:
 1. relazification penalties during benchmark execution (since the compartment will be live)
 2. pathological cases where we get into a bad GC -> relazify -> parse (which bumps gcMallocBytes) -> GC loop

Comment 24

[Till Schneidereit [:till]]

Attachment: Part 1: Remove usages of fun->nonLazyScript

Some preparatory work. Not strictly required with the decision to only re-lazify in empty-stacked compartments, but still nice to have.

Comment 25

[Till Schneidereit [:till]]

Attachment: Part 2: Remove dead IsOriginalScriptFunction

Also not required, obviously, but nice.

Comment 26

[Till Schneidereit [:till]]

Attachment: Part 3: Relazify non-JITted JSFunctions on GC

The meat. Try-servering here: https://tbpl.mozilla.org/?tree=Try&rev=5b5fe7fe03a6

I'm going to wait for the results before asking for review. A *really* nice part of only re-lazifying in empty-stacked compartments is that the patch is very simple again:
7 files changed, 87 insertions(+), 5 deletions(-)

Comment 27

[Jan de Mooij [:jandem]]

Comment on attachment 8355286 [details] [diff] [review]
Part 1: Remove usages of fun->nonLazyScript

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

Looks great.

Comment 28

[Jan de Mooij [:jandem]]

Comment on attachment 8355287 [details] [diff] [review]
Part 2: Remove dead IsOriginalScriptFunction

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

The patch in bug 955838 removed this function as well.

Comment 29

[Till Schneidereit [:till]]

Attachment: Part 3: Relazify non-JITted JSFunctions on GC, v6

After tracking down the source for (, as far as I can tell,) all errors from the try run, this should be good to go.

Try run here, which I hope will be green: https://tbpl.mozilla.org/?tree=Try&rev=672fee3b6cc8

Comment 30

[Luke Wagner [:luke]]

Comment on attachment 8355535 [details] [diff] [review]
Part 3: Relazify non-JITted JSFunctions on GC, v6

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

::: js/src/jsfun.cpp
@@ +529,5 @@
>              MarkScriptUnbarriered(trc, &u.i.s.script_, "script");
> +            // Functions can be relazified if they have a lazyScript, or are
> +            // cloned self-hosted functions that have a name in the first
> +            // private slot.
> +            if (IS_GC_MARKING_TRACER(trc) && !compartment()->hasScriptsOnStack() &&

I think this should be compartment()->hasBeenEntered() for three reasons:
 - theoretically, we could have places in the engine that depend on re-lazification not happening that do not necessarily have scripts on the stack.  One common vector for this to happen is setTimout(builtin), since the JS_CallFunction calls straight into the native.
 - the O(n) activation search happens for every function so, in deeply nested situations, could hurt GC latency
 - the empty-stack browser GCs that I was saying are frequent are careful not to have entered any compartments

Comment 31

[Till Schneidereit [:till]]

(In reply to Luke Wagner [:luke] from comment #30)

Thanks for the explanation - that makes sense. Incorporated in the new try run I had to trigger anyway, because I bungled up the try syntax and didn't actually trigger any tests ...

https://tbpl.mozilla.org/?tree=Try&rev=fe1deee67dd4

Comment 32

[Till Schneidereit [:till]]

I was pretty afraid of the mochitest-browser failures, but they were intermittend - so all green!

Comment 33

[Jan de Mooij [:jandem]]

Comment on attachment 8355535 [details] [diff] [review]
Part 3: Relazify non-JITted JSFunctions on GC, v6

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

Nice work, thanks for not giving up. Doing this for inactive compartments only is indeed a lot simpler. r=me with comments below addressed.

::: js/src/jsfun.cpp
@@ +529,1 @@
>              MarkScriptUnbarriered(trc, &u.i.s.script_, "script");

Would it be possible to do this only if we don't relazify, so that the script can be collected immediately, instead of on the next GC? Maybe ask billm or terrence; this could be a follow-up.

@@ +530,5 @@
> +            // Functions can be relazified if they have a lazyScript, or are
> +            // cloned self-hosted functions that have a name in the first
> +            // private slot.
> +            if (IS_GC_MARKING_TRACER(trc) && !compartment()->hasScriptsOnStack() &&
> +                nonLazyScript()->isRelazifiable() && !compartment()->isSelfHosting && isExtended())

This condition doesn't match the comment: we'll only relazify extended functions and we'll never relazify self-hosted ones. I think it should be:

if (IS_GC_MARKING_TRACER(trc) &&
    !compartment()->hasBeenEntered() &&
    (!isSelfHostedBuiltin() || isExtended()))
{

@@ +1236,5 @@
> +{
> +    JS_ASSERT(isInterpreted());
> +    JS_ASSERT(hasScript());
> +    JS_ASSERT(nonLazyScript()->isRelazifiable());
> +    JS_ASSERT(!compartment()->hasScriptsOnStack());

Don't forget to change this to hasBeenEntered as well.

@@ +1242,5 @@
> +
> +    js::LazyScript *lazy = nonLazyScript()->maybeLazyScript();
> +    u.i.s.lazy_ = lazy;
> +    if (lazy) {
> +        if (!isSelfHostedBuiltin() && lazy->maybeScript() == nonLazyScript())

This could use a comment briefly explaining why we need these two conditions.

@@ +1244,5 @@
> +    u.i.s.lazy_ = lazy;
> +    if (lazy) {
> +        if (!isSelfHostedBuiltin() && lazy->maybeScript() == nonLazyScript())
> +            lazy->resetScript();
> +        u.i.s.lazy_ = lazy;

Nit: you can remove this line, see the "u.i.s.lazy_ = lazy;" before the "if".

::: js/src/vm/SelfHosting.cpp
@@ +889,5 @@
> +                                     ? JSFunction::ExtendedFinalizeKind
> +                                     : fun->getAllocKind();
> +            clone = CloneFunctionObject(cx, fun, cx->global(), kind, TenuredObject);
> +            if (hasName)
> +                clone->as<JSFunction>().setExtendedSlot(0, StringValue(fun->atom()));

Why do we store the atom separately for self-hosted functions, isn't it guaranteed to be the same as clone->atom_? Is fun->atom() == fun->getExtendedSlot(0).toString()->asAtom()? If not, fun->atom() should be changed to fun->getExtendedSlot(...).

@@ +978,5 @@
>      cscript->setFunction(targetFun);
>  
>      JS_ASSERT(sourceFun->nargs() == targetFun->nargs());
> +    // The target function might have been relazified after it's flags changed.
> +    targetFun->setFlags((targetFun->flags() & ~JSFunction::INTERPRETED_LAZY) |

I think we can undo this change now that we only do this if the compartment is not active?

Comment 34

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #33)
> This condition doesn't match the comment: we'll only relazify extended
> functions and we'll never relazify self-hosted ones. I think it should be:
> 
> if (IS_GC_MARKING_TRACER(trc) &&
>     !compartment()->hasBeenEntered() &&
>     (!isSelfHostedBuiltin() || isExtended()))
> {

If isSelfHostedBuiltin() here works, we can also remove JSCompartment::isSelfHosting.

Comment 35

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #33)
> I think it should be:

I just realized I forgot the "nonLazyScript()->isRelazifiable()" part, that should still be in there.

Comment 36

[Till Schneidereit [:till]]

(In reply to Jan de Mooij [:jandem] from comment #33)
> Comment on attachment 8355535 [details] [diff] [review]
> Part 3: Relazify non-JITted JSFunctions on GC, v6
> 
> Review of attachment 8355535 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice work, thanks for not giving up. Doing this for inactive compartments
> only is indeed a lot simpler. r=me with comments below addressed.

Thanks! Before landing, I'll have to do some more work, it seems: I did a lot of measurements yesterday and today, and can't reproduce the great results from comment 0, anymore. :( At the very least, it looks like I'll have to implement bhackett's suggestion from comment 6 for this to be landable without regressing some situations. Will post current results and analysis later.

> >              MarkScriptUnbarriered(trc, &u.i.s.script_, "script");
> 
> Would it be possible to do this only if we don't relazify, so that the
> script can be collected immediately, instead of on the next GC? Maybe ask
> billm or terrence; this could be a follow-up.

Hmm, with the change to only relazify when IS_GC_MARKING_TRACER, this might be possible indeed. I'll check.

> > +            // Functions can be relazified if they have a lazyScript, or are
> > +            // cloned self-hosted functions that have a name in the first
> > +            // private slot.
> > +            if (IS_GC_MARKING_TRACER(trc) && !compartment()->hasScriptsOnStack() &&
> > +                nonLazyScript()->isRelazifiable() && !compartment()->isSelfHosting && isExtended())
> 
> This condition doesn't match the comment: we'll only relazify extended
> functions and we'll never relazify self-hosted ones. I think it should be:
> 
> if (IS_GC_MARKING_TRACER(trc) &&
>     !compartment()->hasBeenEntered() &&
>     (!isSelfHostedBuiltin() || isExtended()))

No, this is correct as-is - it's just not very clear. :( We can (for now, at least) only (re-)lazify clones of self-hosted functions. Since the originals in the self-hosting compartment are not distinguishable from their clones in themselves, we make sure that we're not in the self-hosting compartment. I can move the compartment check into isRelazifiable and properly document all this, however.

> > +    JS_ASSERT(!compartment()->hasScriptsOnStack());
> 
> Don't forget to change this to hasBeenEntered as well.

Thanks

> 
> @@ +1242,5 @@
> > +
> > +    js::LazyScript *lazy = nonLazyScript()->maybeLazyScript();
> > +    u.i.s.lazy_ = lazy;
> > +    if (lazy) {
> > +        if (!isSelfHostedBuiltin() && lazy->maybeScript() == nonLazyScript())
> 
> This could use a comment briefly explaining why we need these two conditions.

Will do

> > +        u.i.s.lazy_ = lazy;
> 
> Nit: you can remove this line, see the "u.i.s.lazy_ = lazy;" before the "if".

Indeed, thanks

> 
> ::: js/src/vm/SelfHosting.cpp
> @@ +889,5 @@
> > +                                     ? JSFunction::ExtendedFinalizeKind
> > +                                     : fun->getAllocKind();
> > +            clone = CloneFunctionObject(cx, fun, cx->global(), kind, TenuredObject);
> > +            if (hasName)
> > +                clone->as<JSFunction>().setExtendedSlot(0, StringValue(fun->atom()));
> 
> Why do we store the atom separately for self-hosted functions, isn't it
> guaranteed to be the same as clone->atom_? Is fun->atom() ==
> fun->getExtendedSlot(0).toString()->asAtom()? If not, fun->atom() should be
> changed to fun->getExtendedSlot(...).

the cloned function can - and almost always does - have a different atom from the original one. The atom stored in the extended slot is used to look up the original function in the self-hosting compartment for cloning. So no to your second question, and this must stay this way.

> > +    // The target function might have been relazified after it's flags changed.
> > +    targetFun->setFlags((targetFun->flags() & ~JSFunction::INTERPRETED_LAZY) |
> 
> I think we can undo this change now that we only do this if the compartment
> is not active?

No, this is not related to whether the compartment is active or not. A function can be marked as a ctor after being cloned over, so the original won't have the SELF_HOSTED_CTOR flag set. This only happens in one fairly obscure place in builtin/Iterator.js, but it does happen.


As I said above, I'll at least incorporate the restoration of lazyScripts into the patch before landing, so I'm afraid you'll have to do another review. I'll try to split things into two patches, though.

Comment 37

[Till Schneidereit [:till]]

Attachment: Part 2: Relazify non-JITted JSFunctions on GC. v7

As jandem pointed out, I had a bug in the conditions under which scripts could be re-lazified. With that fixed, The scenario from comment 0 looks much better again, though not as great as back then.

I had to make a few changes to the patch to make everything work:
- don't re-lazify in debugee compartments. When debugging a compartment, all functions are explicitly un-lazified, because the debugger relies on all scripts being available.
- JSScripts are tightly coupled to the function they're created for. Sometimes, they're shared with other functions, though. Now, if the original function is re-lazified, but not all of its clones are, we have a problem: some parts of the engine start using a function, but then only store its script. Later, they access that script's function to do ... stuff with it. They don't expect that function to be lazy. Theoretically, we could prevent original functions from being lazified as long as there are clones. Practically, that is really hard, because there's no good place to decrease the reference counts when other functions are destroyed. Also, yuck. Thus, we simply un-lazify the original function whenever a function's nonLazyScript() getter is called and we detect that the callee isn't the script's original function. Also kinda yuck, but works nicely.

I documented these things plus a few more gnarly details.


Now, numbers!

The scenario is:
1. start the browser to a restored session containing an about:memory as the active tab and gmail showing a folder with more than one page of messages
2. switch to gmail
3. wait a few seconds for GCs to happen

Before step 2, about 16.000 scripts are created. After minimizing memory usage, scripts take up 7.02MB (3.65MB for individual scripts, plus 3.37MB of runtime-shared memory). The patch doesn't meaningfully affect these numbers.

Step 2 increases the number of created scripts to about 28.000; after step 3, about 8.500 of them are re-lazified - roughly 70% of all scripts created after switching to gmail.

Memory usage after minimizing memory:

without patch: 11.93MB (6.09MB individual scripts, 5.84MB runtime-shared memory)
with patch:     9.73MB (4.89MB individual scripts, 4.84MB runtime-shared memory)

So out of a total possible 4.91MB (11.93MB - 7.02MB from before loading gmail) of memory reduction if no additional scripts were kept around, we realize 2.20MB, or 45%. Well, almost: the memory usage of lazy scripts has increased by 0.53MB from 1.94MB to 2.47MB. We should get rid of that as a follow-up. (As an aside: man, scripts are small!)

Comment 38

[Till Schneidereit [:till]]

and more breakage ... *sigh*

Comment 39

[Nicholas Nethercote [inactive]]

Great results.  In particular, multi-MB wins are really important for FxOS.

Comment 40

[Till Schneidereit [:till]]

Attachment: Part 2: Relazify non-JITted JSFunctions on GC. v8

I just realized I didn't post links to the latest try runs. So, earlier[1], I found out that generators must not be relazified. I guess that makes sense, as generator frames might stay around, and they apparently capture the generator function. Again, that makes sense. That's the intermittend jsreftest error. Why it doesn't reproduce with various shell test cases I tried that seemed to me like they should trigger it, I don't know.

I also have *no* clue what the mochitest-2 thing is about, but I could reproduce it locally before this latest version, but not now, so it seems to also be generators-related.

Let's see how the latest try run[2] turns out, but I'm hopeful.

(In reply to Nicholas Nethercote [:njn] from comment #39)
> Great results.  In particular, multi-MB wins are really important for FxOS.

I agree! I don't, however, know if b2g apps will immediately profit: they might not have lazy scripts, for all I know. But I'll find out, and I definitely know that we can make re-lazification work for them, and all chrome JS.

[1]: https://tbpl.mozilla.org/?tree=Try&rev=48afa82467eb
[2]: https://tbpl.mozilla.org/?tree=Try&rev=60bebc1aadb4

Comment 41

[[:fabrice] Fabrice Desré]

(In reply to Till Schneidereit [:till] from comment #40)
> 
> I agree! I don't, however, know if b2g apps will immediately profit: they
> might not have lazy scripts, for all I know. But I'll find out, and I
> definitely know that we can make re-lazification work for them, and all
> chrome JS.

We are working on getting scripts loaded from the app:// protocol (which is a jar: in disguise) be lazy. And we run a lot of chrome js in b2g so this patch looks already great for us!

Comment 42

[Till Schneidereit [:till]]

(In reply to Fabrice Desré [:fabrice] from comment #41)
> We are working on getting scripts loaded from the app:// protocol (which is
> a jar: in disguise) be lazy. And we run a lot of chrome js in b2g so this
> patch looks already great for us!

Well, almost. :) Currently, chrome JS isn't lazy-parsed, either (at least if nothing changed about that and I just don't know). Chrome scripts have a source policy set that causes us to not save their source, which for the engine means that they aren't lazy-parseable or re-lazifiable. However, it shouldn't be particularly hard to change that: we can get the source, after all. The main reason why it wasn't implemented before is that the vast majority of chrome scripts actually get run at least once. But with re-lazification, that isn't a good argument, anymore.

Comment 43

[[:fabrice] Fabrice Desré]

Till, feel free to help along in bug 944659 ! ;)

Comment 44

[Till Schneidereit [:till]]

Comment on attachment 8355287 [details] [diff] [review]
Part 2: Remove dead IsOriginalScriptFunction

This already happened

Comment 45

[Jan de Mooij [:jandem]]

Comment on attachment 8355924 [details] [diff] [review]
Part 2: Relazify non-JITted JSFunctions on GC. v8

Clearing review until the other problems are fixed.

Comment 46

[Till Schneidereit [:till]]

Attachment: Part 2: Introduce JSScript::nonDelazifyingFunction and use it whereever possible

As discussed, this separates usages of JSScript::function() into ones that expect a non-lazy function (or don't care) and ones that need to make sure that the function is non-lazy. The latter is also possible with the explicit JSScript::ensureNonLazyCanonicalFunction(), which is used at all points where a script is started to be actively used for execution, compilation or inspection.

Comment 47

[Till Schneidereit [:till]]

Attachment: Part 2b

This is a follow-up for part 2. It simply renamines JSScript::function to the more explicit functionNonDelazifying. This way, it's almost impossible to use either of the getters without being aware of their distinction.

I'll fold these two patches together for landing, but splitting out this change from the functionality-changing patch makes reviewing much easier.

Try-servering here:
https://tbpl.mozilla.org/?tree=Try&rev=8bffae725ddf

Comment 48

[Till Schneidereit [:till]]

Attachment: Part 3: Relazify non-JITted JSFunctions on GC. v9

This changed very little since the last version: the remaining crashes from v8 were caused by a script's canonical function being lazy in code that couldn't delazify it. This is prevented by part 2.

The functional changes since the version 6 you reviewed are described in comment 37 and comment 40.

Try-servering here:
https://tbpl.mozilla.org/?tree=Try&rev=6db4792fa890

Comment 49

[Jan de Mooij [:jandem]]

Comment on attachment 8357804 [details] [diff] [review]
Part 2: Introduce JSScript::nonDelazifyingFunction and use it whereever possible

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

::: js/src/frontend/BytecodeCompiler.cpp
@@ +241,5 @@
>  
>      bool savedCallerFun =
>          options.compileAndGo &&
>          evalCaller &&
> +        (evalCaller->functionOrCallerFunction());

Nit: can remove these parentheses now.

::: js/src/jit/CompileInfo.h
@@ +53,5 @@
>  
> +        // The function here can flow in from anywhere so look up the canonical
> +        // function to ensure that we do not try to embed a nursery pointer in
> +        // jit-code. Not delazifying the canonical function here is ok as it is
> +        // guaranteed to have been delazified before compilation started.

As discussed, this isn't true for scripts we inline. I think that's ok though, there's no reason to rely on the original function's script as it's the same as the script we already have.

::: js/src/jsscriptinlines.h
@@ +45,5 @@
>  SetFrameArgumentsObject(JSContext *cx, AbstractFramePtr frame,
>                          HandleScript script, JSObject *argsobj);
>  
> +inline JSFunction *
> +LazyScript::function(JSContext *cx) const {

Nit: { on its own line, same for the 2 functions below.

::: js/src/vm/Interpreter.cpp
@@ +389,5 @@
>      JS_CHECK_RECURSION(cx, return false);
>  
>      SPSEntryMarker marker(cx->runtime());
>  
> +    state.script()->ensureNonLazyCanonicalFunction(cx);

Should we also do this for inline frames (see the pushInlineFrame call below)?

Comment 50

[Jan de Mooij [:jandem]]

Comment on attachment 8357806 [details] [diff] [review]
Part 2b

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

::: js/src/jsscriptinlines.h
@@ +50,2 @@
>      if (function_ && function_->isInterpretedLazy())
>          function_->getOrCreateScript(cx);

I missed this in the previous patch, but we should return nullptr if this fails, and make sure all callers handle this correctly.

Comment 51

[Jan de Mooij [:jandem]]

Comment on attachment 8357809 [details] [diff] [review]
Part 3: Relazify non-JITted JSFunctions on GC. v9

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

::: js/src/jsfun.cpp
@@ +529,1 @@
>              MarkScriptUnbarriered(trc, &u.i.s.script_, "script");

Should we move this into an |else| of the |if| below? To make sure the script can be destroyed immediately instead of waiting for the next GC. I wonder how much this impacts the memory usage numbers.

@@ +529,2 @@
>              MarkScriptUnbarriered(trc, &u.i.s.script_, "script");
> +            // Functions can be relazified under the following conditions:

Great comment!

@@ +532,5 @@
> +            //   debugged
> +            // - they are not in the self-hosting compartment
> +            // - they aren't generators
> +            // - they don't have JIT code attached
> +            // - they don't have child functions

Just curious, do you know how many scripts have child functions for, say, Gmail?

@@ +1271,5 @@
> +        MarkLazyScriptUnbarriered(trc, &u.i.s.lazy_, "lazyScript");
> +    } else {
> +        JS_ASSERT(isSelfHostedBuiltin());
> +        JS_ASSERT(isExtended());
> +        JS_ASSERT(getExtendedSlot(0).isString() && getExtendedSlot(0).toString()->isAtom());

Nit: toString() asserts isString(), so you could remove the first part.

Comment 52

[Till Schneidereit [:till]]

(In reply to Jan de Mooij [:jandem] from comment #51)
> >              MarkScriptUnbarriered(trc, &u.i.s.script_, "script");
> 
> Should we move this into an |else| of the |if| below? To make sure the
> script can be destroyed immediately instead of waiting for the next GC. I
> wonder how much this impacts the memory usage numbers.

We totally should, and I did for earlier patches. This change got lost somehow, so I reintroduced it in the (hopefully) final patch. Numbers were largely unchanged at least for gmail: it triggers multiple GCs during loading, the first of which re-lazifies the bulk of functions, which the second then collects the scripts for. Obviously, it's much better to do that in one step.


> > +            // - they don't have child functions
> 
> Just curious, do you know how many scripts have child functions for, say,
> Gmail?

A few hundred. Certainly worth looking into as a follow-up. We would have to make sure the outer function is delazified whenever a child function is used, though, as there are too many places that assume the outer function's script to be available, with some of them off-mainthread.


Updated patches try-servering here:
https://tbpl.mozilla.org/?tree=Try&rev=b5d817b54d9b

Comment 53

[Till Schneidereit [:till]]

Landed with nits and final crashes addressed. R+ on the changes from jandem via IRC.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2984d53fe0cf
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0292b2de36c0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/508ad49212c0

\o/

Comment 54

[Till Schneidereit [:till]]

Oh, and here's a very shiny last try run:
https://tbpl.mozilla.org/?tree=Try&rev=18c3eae0279f

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2984d53fe0cf
https://hg.mozilla.org/mozilla-central/rev/0292b2de36c0
https://hg.mozilla.org/mozilla-central/rev/508ad49212c0