Status

()

Core
JavaScript Engine
RESOLVED INVALID
4 years ago
4 years ago

People

(Reporter: terrence, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

4 years ago
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.
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.
(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.
(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.
Flags: needinfo?(jdemooij)
(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
Flags: needinfo?(jdemooij)
(Reporter)

Comment 5

4 years ago
Uhg, I guess this is infeasible and we'd best just keep the API as it is.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → INVALID

Comment 6

4 years ago
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.
You need to log in before you can comment on or make changes to this bug.