911293 - Use constants to prevent the generation of code.

Status: NEW

(Core :: JavaScript Engine, enhancement, P3)

Description

[Nicolas B. Pierron [:nbp] {away}]

The idea, is that a lot of code might have debug statements as part of the code that they ship.  Debug statement, especially assertions are interesting for finding security bugs.

When code is in release mode, developeprs might not want to get the arguments evaluated.  One way to do that is to use the eager evaluation of the && operator by using statements such as:

   DEBUG && assert(longComputation(), function () { capturesVariables(a,b,c); })

Sadly, we might want to get rid of most of this code[1] to prevent having additional bytecode or even additional captured variables.

Jason mentioned that we almost handle this case in the FoldConstant phase[2] of the frontend.


[1]
js> const DEBUG = false;
js> function f() { foo(); DEBUG && bar(); baz(); }
js> dis(f)
flags:
loc     op
-----   --
main:
00000:  callgname "foo"
00005:  undefined
00006:  notearg
00007:  call 0
00010:  pop
00011:  getgname "DEBUG"
00016:  and 32 (+16)
00021:  pop
00022:  callgname "bar"
00027:  undefined
00028:  notearg
00029:  call 0
00032:  pop
00033:  callgname "baz"
00038:  undefined
00039:  notearg
00040:  call 0
00043:  pop
00044:  stop


[2] http://dxr.mozilla.org/mozilla-central/source/js/src/frontend/FoldConstants.cpp#l465

Comment 1

[Jason Orendorff [:jorendorff]]

Here are some optimizations that FoldConstants already does along these lines:

    if (false) do_something();  // this is stripped entirely
    if (false && something) ;   // this is stripped entirely
    if (true && something) ;    // the "1 &&" is stripped
    if (true || something) f(); // the "if (true || something)" is stripped

You can confirm these in the JS shell using dis(), as shown in comment 0.

Here is how to extend these optimizations to make assertions very nearly zero-cost:

1. Modify Boolish (in FoldConstants.cpp) to handle PNK_NAME nodes, in particular
   so that it correctly identifies the expression `DEBUG` as "Falsy" when the
   binding for DEBUG in the current scope is a constant declaration of the form
   `const DEBUG = false;`.

   This will complicate Boolish either a little, or rather more than a little;
   I'm not sure which.  It knows nothing of scopes so far.

2. Add a fourth SyntacticContext constant: Ignored, for expressions that occur
   in syntactic contexts such that their value is discarded. In this code:

            SyntacticContext kidsc =
                pn->isKind(PNK_NOT)
                ? SyntacticContext::Condition
                : pn->isKind(PNK_DELETE)
                ? SyntacticContext::Delete
                : SyntacticContext::Other;

   add more cases, such that if pn->isKind(PNK_SEMI) or PNK_VOID then
   kidsc is SyntacticContext::Ignored.

   I can explain what SyntacticContext means if that is baffling...

3. Enable Boolish-related constant folding when sc is Ignored. (It is currently
   enabled only when sc is Condition.)

The FoldConstants code is rather quaint and cheerful to work with.  So this sounds like fun to me.  But I really can't spare the time for it!

Comment 2

[Jason Orendorff [:jorendorff]]

(In reply to Jason Orendorff [:jorendorff] from comment #1)
> Here is how to extend these optimizations to make assertions very nearly
> zero-cost:

I say "very nearly" because there is one way a stripped-out assertion could still slow down the code.

If an assertion contains anything that would inhibit optimizations (for example, if it uses direct eval, or contains any closures that touch locals or arguments in enclosing scopes) those optimizations may remain inhibited even if the offending code is stripped out. (FoldConstants may simply happen too late.)

But I think this should be close enough. Very few assertions will deoptimize anything.

Comment 3

[Wei Wu [:wuwei UTC+8]]

I'm glad to work on this bug after I finished the LIR reorder optimization. You can assign it to me if it is not time urgent.

Comment 4

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Jason Orendorff [:jorendorff] from comment #1)
> Here is how to extend these optimizations to make assertions very nearly
> zero-cost:

Thanks for describing this :)

(In reply to Wei Wu [:wuwei UTC+8] from comment #3)
> I'm glad to work on this bug after I finished the LIR reorder optimization.
> You can assign it to me if it is not time urgent.

When the LIR reordering / GSoC is complete, I can assign you to this bug.  There is no need to prevent any body from taking this bug in the mean time.

I wish we could get this bugs addressed before gecko 27 such as Gaia can start relying on it for the version 1.2 of B2G.

Thanks.

Comment 6

[Nicolas B. Pierron [:nbp] {away}]

Jason, would you mentor this Bug?

I guess this might interest Iseki to implement it as he already used FoldConstant for the string concatenations.

Comment 7

[Masaya Iseki[:isk](UTC+9)]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> I guess this might interest Iseki to implement it as he already used
> FoldConstant for the string concatenations.

Thanks :)

I'm interested in this bug.

Comment 8

[Masaya Iseki[:isk](UTC+9)]

I tried to fix this bug.
I have a question.
How to identify 'DEBUG' as 'false' or 'true'?
We can't identify the value of variable in parse phase.
I think constant value is the same.

I read Comment#2 , but I can't understand well.

Comment 9

[Jason Orendorff [:jorendorff]]

(In reply to iseki.m.aa from comment #8)
> I tried to fix this bug.
> I have a question.
> How to identify 'DEBUG' as 'false' or 'true'?

There are two situations where we can be sure of this.

1. A previous script has already declared and executed `const DEBUG = <expression>;`
   in this global.

2. The current script contains `const DEBUG = <expression>;` and will execute it
   before evaluating the code that we are trying to constant-fold.

Once a constant declaration actually executes, the value of the constant can't change.

Comment 10

[Masaya Iseki[:isk](UTC+9)]

(In reply to Jason Orendorff [:jorendorff] from comment #9)

Thanks for your comment.

I have two question.

1. How to get constant value in FoldConstants.cpp?
2. How to execute code in FoldConstants.cpp?

Comment 11

[Jason Orendorff [:jorendorff]]

(In reply to iseki.m.aa from comment #10)
> 1. How to get constant value in FoldConstants.cpp?

The parser is an argument to FoldConstants(). It has a reference to the global object somewhere.

> 2. How to execute code in FoldConstants.cpp?

Don't! You can't do this safely.

We can only optimize this if we know that the constant declaration *will* be executed before the code we are trying to constant-fold. That is true for this script:

    const DEBUG = false;
    DEBUG && assert(1 + 1 == 2, "math should work");

but not for this one:

    const DEBUG = f("DEBUG");
    function f(s) {
        DEBUG && assert(typeof s === "string");  // runs before DEBUG is initialized!
        return s in environment;
    }

It is generally hard to tell which statements run in which order.

Comment 12

[Masaya Iseki[:isk](UTC+9)]

(In reply to Jason Orendorff [:jorendorff] from comment #11)

Thanks you for your comment.

> > 1. How to get constant value in FoldConstants.cpp?
> 
> The parser is an argument to FoldConstants(). It has a reference to the
> global object somewhere.

Sorry, I can't find how to get constant value.
I found LexicalLookup. But this method can't get the value.

Comment 13

[Jason Orendorff [:jorendorff]]

All right. To be honest, this is pretty tricky. It might be good to unassign yourself from this bug and work on other bugs until you're ready for this one.

Comment 14

[Masaya Iseki[:isk](UTC+9)]

I understand.
Thanks for your mentoring.

Comment 15

[Masaya Iseki[:isk](UTC+9)]

JSObject::lookupProperty[1] can get value in FoldConstants.cpp?
I define like below
Boolish(ParseNode *pn,ExclusiveContext *cx)
...
       RootedShape prop(cx);
       RootedObject varobj(cx);
       RootedObject obj2(cx);
       PropertyName *name = nullptr;
       name = pn->pn_atom->asPropertyName();
       if (!JSObject::lookupProperty(cx, varobj, name, &obj2, &prop))
            return Falsy;

The first argument of JSObject::lookupProperty is JSContext[2] but cx is ExclusiveContext.
So this code can't pass compiler.
Is there way to pass compiler?

[1]http://dxr.mozilla.org/mozilla-central/source/js/src/jsobj.h#946
[2]http://dxr.mozilla.org/mozilla-central/source/js/src/jscntxt.h#412

Comment 16

[Jason Orendorff [:jorendorff]]

JSObject::lookupProperty can trigger side effects, so it shouldn't be called from the compiler. Instead, use js::LookupPropertyPure, declared in jsobj.h. It doesn't require a cx at all.

Comment 17

[Nicolas B. Pierron [:nbp] {away}]

Hi isk, are you interested in continuing your investigation on this bug?

Comment 18

[Masaya Iseki[:isk](UTC+9)]

Hi nbp.
Sorry, I didn't notice jorendorff's answer.

I'm interested in solving this bug but may be take time because recently I'm little busy.

Comment 19

[Masaya Iseki[:isk](UTC+9)]

Attachment: bug911293.patch

Sorry to delay.

This patch is WIP and cause assertion failure at |lastProperty|[1]. The reason is that the first arguments(obj) of |LookupPropertyPure| is not appropriate. This JSObject is just allocated. How to get appropriate JSObject?

[1]http://dxr.mozilla.org/mozilla-central/source/js/src/vm/ObjectImpl.h#546

Comment 20

[Jason Orendorff [:jorendorff]]

Comment on attachment 8428662 [details] [diff] [review]
bug911293.patch

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

Argh, sorry my previous comment didn't work. I should have foreseen this.

Brian, do you know what iseki should do here?

Comment 21

[Brian Hackett [Laid off!]]

Comment on attachment 8428662 [details] [diff] [review]
bug911293.patch

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

When we're in FoldConstants I don't think we've made any attempt to fixup parse nodes with the actual op they should use; that is done in BindNameToSlot when we're emitting bytecode.  There's a lot going on in that function and I don't know if it's possible to call it during FoldConstants since it uses the BytecodeEmitter.

Comment 22

[Wei Wu [:wuwei UTC+8]]

Hi isk, are you still working on this bug?

Comment 23

[Wei Wu [:wuwei UTC+8]]

Hi Nicolas, is this feature still helpful?

Comment 24

[Nicolas B. Pierron [:nbp] {away}]

(In reply to Wei Wu [:w :wuwei UTC+8] from comment #23)
> Hi Nicolas, is this feature still helpful?

Yes, I think it is.

Comment 25

[BugBot [:suhaib / :marco]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 26

[Matthew Gaudet (he/him) [:mgaudet]]

(We should verify Fold Constants didn't already get smarter)