1099454 - Remove noScriptRval

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

There is a ton of infrastructure around this field, both inside and outside the engine, but not for any good reason. Naively, it should only be epsilon slower to produce a value instead of undefined and it has basically no impact once you have a JIT. At least, this would be true, except for one wrinkle: EmitStatement uses this flag to produce or not produce a value for every. single. statement.

Obviously, the combinatorial impact here is enormously more than it should be. Waldo and I were not able to dream up any harebrained situation where these side effects could even be exposed, so it seems like it would be enormously better to fix EmitStatement to only keep the rval in the top-level script so that we can just unconditionally produce noScriptRval=false at no cost.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This would be pretty awesome if we can do it with no perf loss.  Note that we've had issues in the past with not being able to ion-compile scripts compiled without noScriptRval, due to it using a jsop that Ion can't handle.  See bug 834785.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Please do not ask for reviews for a bit [:bz] from comment #1)
> This would be pretty awesome if we can do it with no perf loss.  Note that
> we've had issues in the past with not being able to ion-compile scripts
> compiled without noScriptRval, due to it using a jsop that Ion can't handle.
> See bug 834785.

Fortunately this is no longer true and Ion can compile those ops now (bug 890722).

(In reply to Terrence Cole [:terrence] from comment #0)
> so it seems like it would be
> enormously better to fix EmitStatement to only keep the rval in the
> top-level script so that we can just unconditionally produce
> noScriptRval=false at no cost.

EmitStatement only does the SETRVAL thing for eval/global/JS_Execute scripts. It'd be a bit unfortunate to have a SETRVAL after every expression statement in global scripts though, but it's annoying to fix. For instance the following script has to produce "foo":

    for (var i=0; i<100; i++) {
        "foo";
        if (i >= 0)
            continue;
        "bar"
    }

The analysis would have to be like this: if a block has N consecutive *expression* statements, SETRVAL is only necessary for the last one.

But it's probably OK to just emit SETRVAL after every expression statement. I think it doesn't really matter for Baseline/Ion performance but we should measure.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> The analysis would have to be like this: if a block has N consecutive
> *expression* statements, SETRVAL is only necessary for the last one.

Hm, when we emit a statement, we have access to the next one so we can just check if it's an expression statement. I'll do this next week.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> The analysis would have to be like this: if a block has N consecutive
> *expression* statements, SETRVAL is only necessary for the last one.

Thinking about this more, even that is not true:

js> function throwErr() { throw 1; }
js> try { 3; throwErr(); } catch(e) {}
3

Comment 5

[Terrence Cole [:terrence]]

Uhg, I guess this is infeasible and we'd best just keep the API as it is.

Comment 6

[Jeff Walden [:Waldo]]

But aren't try-statements like that, at the end of a top-level script, really the exceptional case?  Handling only try-statements more slowly is not a dealbreaker, particularly given that try-statements already incur some extra cost.  I'm not convinced this is impossible.