Bug 142944 – [ES6] implement block scoping to enable 'let'

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

The spec:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-let-and-const-declarations

[Saam Barati]

So far it's a total hack, but I've got this working:

```
"use strict";

function truth() {
    return true;
}
function foo() {
    let x = "it works!";
    print(x);
    if (truth()) {
        let x = "it works again!";
        print(x);
    }
    print(x);
}
foo();
```

Going to post a WIP patch as hackiness subdues.

[Saam Barati]

Created attachment 249945 [details]
WIP

It begins.

Currently only working for functions, not global code.
Stuff like this is working:

```
function foo() {
    let x = 20;
    print(x + 10);
    var arr = [];
    function capArr() { return arr; }

    for (var i = 0; i < 5; ++i) {
        let x = i;
        print("inner: " + x);
        arr.push(function() { let y; print(x); function baz() { return y; }});
    }

    for (var i = 0; i < arr.length; ++i)
        arr[i]();
}
```

[Saam Barati]

(In reply to comment #2)
> Created attachment 249945 [details]
> WIP
> 
> It begins.
> 
> Currently only working for functions, not global code.
> Stuff like this is working:
> 
> ```
> function foo() {
In strict mode, that is.

[Saam Barati]

Created attachment 249961 [details]
WIP

defined new op code.

[Saam Barati]

Created attachment 250169 [details]
work in progress

This is my test file and this stuff is working:

```
"use strict";

let globalLet = "helloWorld";
assert(globalLet === "helloWorld");
function captureGlobalLet() { return globalLet; }
var arrOfFuncs = [];
for (var i = 0; i < 100; i++) {
    let globalLet = "inner";
    assert(globalLet === "inner");
    let inner = i;
    arrOfFuncs.push(function() { return inner; });
}
assert(globalLet === "helloWorld");
for (var i = 0; i < arrOfFuncs.length; i++)
    assert(arrOfFuncs[i]() === i);



function truth() {
    return true;
}
noInline(truth);

function assert(cond) {
    if (!cond)
        throw new Error("broke assertion");
}
noInline(assert);


;(function() {

function foo() {
    let x = 20;
    
    if (truth()) {
        let thingy = function() { 
            x = 200;
            return x; 
        };
        noInline(thingy);
        thingy();
    }

    return x;
}

for (var i = 0; i < 1000; i++) {
    assert(foo() === 200);
}

})();


;(function() {

function foo() {
    let x = 20;
    let y = 40;
    assert(x === 20);
    assert(y === 40);
    if (truth()) {
        let x = 50;
        let y = 60;
        assert(x === 50);
        assert(y === 60);
    }
    assert(x === 20);
    assert(y === 40);
}

for (var i = 0; i < 1000; i++) {
    foo();
}

})();


;(function() {

function foo() {
    let x = 20;
    let y = 40;
    function captureX() { return x; }
    assert(x === 20);
    assert(y === 40);
    if (truth()) {
        let x = 50;
        let y = 60;
        assert(x === 50);
        assert(y === 60);
    }
    assert(x === 20);
    assert(y === 40);
}

for (var i = 0; i < 1000; i++) {
    foo();
}

})();


// FIXME: currently, captured let variables are allocated in one activation
// above the base activation for functions. This causes the initialization
// of captureAll to grab the wrong scope register as its scope so it doesn't
// 'see' the let variables 'x' and 'y'. Fix this by having the initial block
// statement of function nodes and their respective lexical aspects to be lowered
// into functionNode itself so hoisted functions have access to these variables.
//;(function() {
//
//function foo() {
//    let x = 20;
//    let y = 40;
//    function captureAll() { return x + y; }
//    noInline(captureAll);
//    assert(x === 20);
//    assert(y === 40);
//    assert(captureAll() === 60);
//    if (truth()) {
//        let x = 50;
//        assert(x + y === 90);
//        //assert(y === 40);
//        //assert(captureAll() === 60);
//    }
//    assert(x === 20);
//    assert(y === 40);
//}
//
//for (var i = 0; i < 1000; i++) {
//    foo();
//}
//
//})();

;(function() {
function foo() {
    let x = 20;
    let y = 40;
    var captureAll = function() { return x + y; }
    assert(x === 20);
    assert(y === 40);
    assert(captureAll() === 60);
    if (truth()) {
        let x = 50;
        assert(x + y === 90);
        assert(y === 40);
        assert(captureAll() === 60);
    }
    assert(x === 20);
    assert(y === 40);
}

for (var i = 0; i < 1000; i++) {
    foo();
}

})();

;(function() {
function foo() {
    let x = 20;
    let y = 40;
    var captureAll = function() { return x + y; }
    assert(x === 20);
    assert(y === 40);
    assert(captureAll() === 60);
    if (truth()) {
        let x = 50;
        let secondCaptureAll = function() { return x + y; };
        assert(x + y === 90);
        assert(secondCaptureAll() === 90);
    }
    assert(x === 20);
    assert(y === 40);
}

for (var i = 0; i < 1000; i++) {
    foo();
}

})();
```

[Saam Barati]

Created attachment 250230 [details]
WIP

beginning to implement TDZ.

Need to go through NodesCodegen and insert TDZ checks where needed, and I need
to add some machinery for TDZ checks inside a nested function:
```
function foo() {
    capture();
    let baz = 20;
    function capture() { return baz; }
}
```

[Saam Barati]

Created attachment 250533 [details]
work in progres

A lot of the necessary infrastructure is in place now. I need to go through
various parts of NodesCodegen and the Parser to hook into this functionality. 
i.e, need to make 'const'/'class' nodes lexically declared. I need to emit TDZ
checks where necessary, etc, etc.

Also, there is one key piece of infrastructure not in place for TDZ implementation, and I'm not sure the
best way to handle it. Let me know if anyone has a recommendation on how to solve it.
The problem is such:

```
function foo() {
    shouldThrow();
    let x = 20;
    function shouldThrow() { return x; }
}
```

Basically, we need a way to determine if a variable is under TDZ in an outer
function scope when parsing/compiling an inner function scope. This would allow
us to generate some flags on particular AST nodes to have them emit TDZ checks 
for variable that aren't locals.

[Saam Barati]

Created attachment 250684 [details]
WIP

working in loops.

[Saam Barati]

Created attachment 250714 [details]
WIP

Rebased yesterday.

[Saam Barati]

Created attachment 250715 [details]
perf tests

[Oliver Hunt]

Comment on attachment 250714 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=250714&action=review

> Source/JavaScriptCore/parser/Nodes.h:1088
> +        bool m_isInVariableDeclaration;

I'd prefer that you use an enum type for this - enum class ResolveKind, or some such

[Oliver Hunt]

So looping questions, could you look at:

for (let thing = thing ....) // all enumeration types
for (let thing = eval("thing") ....) // all enumeration types
for (let thing = function() { return thing } ....) // all enumeration types

Do we support defaults in destructuring assignment yet?

[Saam Barati]

(In reply to comment #12)
> So looping questions, could you look at:
> 
> for (let thing = thing ....) // all enumeration types
> for (let thing = eval("thing") ....) // all enumeration types
> for (let thing = function() { return thing } ....) // all enumeration types

Good call. I'm mis-compiling `for (let thing of thing) ;`

>
> Do we support defaults in destructuring assignment yet?
Not yet, I've opened this bug for that:
https://bugs.webkit.org/show_bug.cgi?id=142679

[Saam Barati]

Created attachment 251081 [details]
WIP

This patch is really close to being done. It's passing both my tests and JSC's tests.
There is one last thing that doesn't work:

```
function foo() {
    let x = 20;
    try {
        throw "error";
    } catch(e) {
        assert(x === 20);
    }
}
```
It seems that BytecodeGenerator will increment m_localScopeDepth both for
'try/catch' and 'with' statements. What's the reason for this? We don't do
any variable resolution when we have 'm_localScopeDepth > 0'. Why is this?
Why are both 'try/catch' and 'with' lumped together into the same scope
depth? I think we should always be able to statically resolve where a variable
will be if we're just dealing with try/catch scopes, right? (Or am I totally
off on this?)

My plans of finishing this patch up are:
1. Make the above example work.
2. clean up the code a bit and prune the FIXMEs
3. Add the tests I have to the patch.

[Saam Barati]

(In reply to comment #14)
> Created attachment 251081 [details]
> WIP
> 

> It seems that BytecodeGenerator will increment m_localScopeDepth both for
> 'try/catch' and 'with' statements. What's the reason for this? We don't do
> any variable resolution when we have 'm_localScopeDepth > 0'. Why is this?
> Why are both 'try/catch' and 'with' lumped together into the same scope
> depth? I think we should always be able to statically resolve where a
> variable
> will be if we're just dealing with try/catch scopes, right? (Or am I totally
> off on this?)
> 

Maybe it's best to land without this and fix in another patch?
It seems like some other push/pop scope mechanisms can be reduced
to push_lexical_scope and pop_lexical_scope.

[WebKit Commit Bot]

Attachment 251081 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:317:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:317:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:318:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:318:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:319:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:319:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:320:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:320:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:321:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:321:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:322:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:322:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:323:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:323:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:324:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:325:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:325:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:326:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:326:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/parser/NodeConstructors.h:860:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/parser/NodeConstructors.h:870:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/parser/NodeConstructors.h:875:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1211:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1332:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1334:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1337:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:393:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:755:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 29 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 251130 [details]
WIP

exception handling done.

[WebKit Commit Bot]

Attachment 251130 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:317:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:317:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:318:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:318:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:319:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:319:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:320:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:320:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:321:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:321:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:322:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:322:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:323:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:323:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:324:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:325:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:325:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:326:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:326:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/parser/NodeConstructors.h:860:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/parser/NodeConstructors.h:870:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/parser/NodeConstructors.h:875:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:495:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:501:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1251:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1257:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1345:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1347:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1350:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:401:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:764:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 32 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 251130 [details]
WIP

Attachment 251130 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6541564956377088

New failing tests:
js/basic-strict-mode.html
js/mozilla/strict/13.1.html

[Build Bot]

Created attachment 251133 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Saam Barati]

Created attachment 251136 [details]
WIP

remove print statements from previous patch

[WebKit Commit Bot]

Attachment 251136 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:317:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:317:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:318:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:318:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:319:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:319:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:320:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:320:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:321:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:321:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:322:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:322:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:323:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:323:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:324:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:325:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:325:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:326:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/runtime/SymbolTable.h:326:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/parser/NodeConstructors.h:860:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/parser/NodeConstructors.h:870:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/parser/NodeConstructors.h:875:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1335:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:393:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:756:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 26 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 251409 [details]
patch

Getting really close. What's left:

1. Throw syntax errors for repetitive let declarations and write tests for these.
2. Fill in check TDZs in NodesCodegen, (still), and add the necessary tests.
3. Fix a bug where the lexical scope is constant folded under DFG-eager tests when it shouldn't be.

[WebKit Commit Bot]

Attachment 251409 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:1974:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/parser/NodeConstructors.h:869:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/parser/NodeConstructors.h:879:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/parser/NodeConstructors.h:884:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1335:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:393:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:756:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 8 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

(In reply to comment #23)
> Created attachment 251409 [details]
> patch
> 
> Getting really close. What's left:
> 
> 1. Throw syntax errors for repetitive let declarations and write tests for
> these.
> 2. Fill in check TDZs in NodesCodegen, (still), and add the necessary tests.
> 3. Fix a bug where the lexical scope is constant folded under DFG-eager
> tests when it shouldn't be.

Add one more:
4. Make global functions that reference 'let' variables declared in the global scope work.

[Saam Barati]

Created attachment 251653 [details]
WIP

[Saam Barati]

Created attachment 251913 [details]
rebased patch

[WebKit Commit Bot]

Attachment 251913 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/NodeConstructors.h:914:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/parser/NodeConstructors.h:924:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/parser/NodeConstructors.h:929:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1381:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1401:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1663:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:407:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:544:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:776:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 10 in 62 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 252074 [details]
patch

pretty close to being done.
Just waiting on blocking patches.

[Saam Barati]

(In reply to comment #25)
> (In reply to comment #23)
> > Created attachment 251409 [details]
> > patch
> > 
> > Getting really close. What's left:
> > 
> > 1. Throw syntax errors for repetitive let declarations and write tests for
> > these.
Done.

> > 2. Fill in check TDZs in NodesCodegen, (still), and add the necessary tests.
Done.

> > 3. Fix a bug where the lexical scope is constant folded under DFG-eager
> > tests when it shouldn't be.
Done.

> 
> Add one more:
> 4. Make global functions that reference 'let' variables declared in the
> global scope work.
And done but waiting on "depends on" patches.

[WebKit Commit Bot]

Attachment 252074 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:775:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 62 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 253066 [details]
patch

[WebKit Commit Bot]

Attachment 253066 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1365:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:774:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 63 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 253066 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253066&action=review

> Source/JavaScriptCore/bytecode/EvalCodeCache.h:56
> +            JSScope::collectVariablesUnderTDZ(scope, variablesUnderTDZ);

One implementation note:

Because direct eval will be implemented within the current scope, and indirect eval won't. We could change this implementation so that we don't collect these variables
from the Scope, but instead, have a mapping from byte code index to variables under TDZ because the byte code generator will know of all direct eval calls.

> Source/JavaScriptCore/parser/ParserModes.h:87
> +typedef HashSet<RefPtr<StringImpl>, IdentifierRepHash> IdentifierSet;

I'm going to open a bug to abstract this away into it's own class.
We will need this to support const. I also think it will clean up
passing around IdentifierSet twice to constructors for LexicalNode.

[Saam Barati]

(In reply to comment #34)
> Comment on attachment 253066 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253066&action=review
> 
> > Source/JavaScriptCore/bytecode/EvalCodeCache.h:56
> > +            JSScope::collectVariablesUnderTDZ(scope, variablesUnderTDZ);
> 
> One implementation note:
> 
> Because direct eval will be implemented within the current scope, and
> indirect eval won't. We could change this implementation so that we don't
> collect these variables
> from the Scope, but instead, have a mapping from byte code index to
> variables under TDZ because the byte code generator will know of all direct
> eval calls.

Should read: "direct eval will be executed"

[Saam Barati]

Comment on attachment 253066 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253066&action=review

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:794
> +    bineq TagOffset[cfr, t1, 8], EmptyValueTag, .opNotTDZ

Fixed locally:
bineq t1, EmptyValueTag, .opNotTDZ

[Saam Barati]

Created attachment 253070 [details]
patch

[WebKit Commit Bot]

Attachment 253070 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1365:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:774:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 63 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Oh man, that's a juicy patch.  Looking now.

[Filip Pizlo]

Comment on attachment 253070 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253070&action=review

> Source/JavaScriptCore/dfg/DFGClobberize.h:321
> +        SymbolTable* table;
> +        if (node->child2())
> +            table = node->child2()->castConstant<SymbolTable*>();
> +        else
> +            table = graph.symbolTableFor(node->origin.semantic);

You could get rid of these conditionals if you made the outermost CreateActivation also have a child2, and that child2 happened to point to graph.symbolTableFor(origin).

[Saam Barati]

Created attachment 253115 [details]
patch

Add TDZ check for destructuring assignment expressions:
```
[x] = [10];
let x;
```

[Saam Barati]

(In reply to comment #40)
> Comment on attachment 253070 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253070&action=review
> 
> > Source/JavaScriptCore/dfg/DFGClobberize.h:321
> > +        SymbolTable* table;
> > +        if (node->child2())
> > +            table = node->child2()->castConstant<SymbolTable*>();
> > +        else
> > +            table = graph.symbolTableFor(node->origin.semantic);
> 
> You could get rid of these conditionals if you made the outermost
> CreateActivation also have a child2, and that child2 happened to point to
> graph.symbolTableFor(origin).

Yeah that's cleaner. Will make this change.

[Geoffrey Garen]

Comment on attachment 253115 [details]
patch

Looks like this needs rebasing.

[Geoffrey Garen]

Comment on attachment 253115 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253115&action=review

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1993
> +                    iter->value.disableWatching();

Does this mean that the watchpoint optimization will never work for let variables -- even in cases when it would be profitable? Does this make let slower than var?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:594
> +RegisterID* BytecodeGenerator::newLexicalVariable()

I would call this "newLet" or "newBlockScopeVariable". "Lexical" is kind of a nebulous term.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:710
> +        Vector<SymbolTableStackEntry> m_symbolTableStack;

What's the plan for making the stack of symbol tables available to the debugger?

I don't think the debugger can work with this design: I think the stack needs to be a run-time data structure, not a compile-time data structure.

Perhaps the debugger will work naturally if you force each block scope to heap-allocate its variables. That will make the debugger much slower than it already is :(.

[Saam Barati]

(In reply to comment #44)
> Comment on attachment 253115 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253115&action=review
> 
> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1993
> > +                    iter->value.disableWatching();
> 
> Does this mean that the watchpoint optimization will never work for let
> variables -- even in cases when it would be profitable? Does this make let
> slower than var?
> 
For captured 'let' variables, yes, it will prevent optimizations and be slower. 
It probably makes sense to make the TDZ flag value not be empty JSValue
at some point so we don't run into this problem. 

Also, I think 'let' will always be slower than 'var' by nature 
of the TDZ. It's kind of crappy that this is the case because
'let' is a lot nicer than 'var'. The DFG should eliminate almost all
TDZ checks, so for hot enough code, it probably won't be that
big of a performance hit.

> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:594
> > +RegisterID* BytecodeGenerator::newLexicalVariable()
> 
> I would call this "newLet" or "newBlockScopeVariable". "Lexical" is kind of
> a nebulous term.
I like "newBlockScopeVariable".

> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:710
> > +        Vector<SymbolTableStackEntry> m_symbolTableStack;
> 
> What's the plan for making the stack of symbol tables available to the
> debugger?
> 
> I don't think the debugger can work with this design: I think the stack
> needs to be a run-time data structure, not a compile-time data structure.
Doesn't JSScope act as a defacto stack? I'm not well versed on how the
debugger works so I'm not sure where this breaks that model. What specifically
does this hinder and is there no way to reconcile this at compile time?
I'll spend some time reading through the debugger code.

> 
> Perhaps the debugger will work naturally if you force each block scope to
> heap-allocate its variables. That will make the debugger much slower than it
> already is :(.
Yeah we should avoid this if possible.

[Geoffrey Garen]

> > > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1993
> > > +                    iter->value.disableWatching();
> > 
> > Does this mean that the watchpoint optimization will never work for let
> > variables -- even in cases when it would be profitable? Does this make let
> > slower than var?
> > 
> For captured 'let' variables, yes, it will prevent optimizations and be
> slower. 
> It probably makes sense to make the TDZ flag value not be empty JSValue
> at some point so we don't run into this problem. 
> 
> Also, I think 'let' will always be slower than 'var' by nature 
> of the TDZ. It's kind of crappy that this is the case because
> 'let' is a lot nicer than 'var'. The DFG should eliminate almost all
> TDZ checks, so for hot enough code, it probably won't be that
> big of a performance hit.

Are there any cases where we can rescue the constant closure optimization? It's a really important optimization, especially in asm.js, and we'll suffer huge losses if people mass switch from var to let.

> > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:594
> > > +RegisterID* BytecodeGenerator::newLexicalVariable()
> > 
> > I would call this "newLet" or "newBlockScopeVariable". "Lexical" is kind of
> > a nebulous term.
> I like "newBlockScopeVariable".
> 
> > 
> > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:710
> > > +        Vector<SymbolTableStackEntry> m_symbolTableStack;
> > 
> > What's the plan for making the stack of symbol tables available to the
> > debugger?
> > 
> > I don't think the debugger can work with this design: I think the stack
> > needs to be a run-time data structure, not a compile-time data structure.
> Doesn't JSScope act as a defacto stack?

JSScope does act as a stack. But you aren't emitting a JSScope for every let scope. You're only emitting a JSScope if the let scope is captured. There's the rub. You can either emit a JSScope for every let scope or find a way to represent the compile-time-only scope stack in a way that the debugger can consume.

[Saam Barati]

(In reply to comment #46)
> > > > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1993
> > > > +                    iter->value.disableWatching();
> > > 
> > > Does this mean that the watchpoint optimization will never work for let
> > > variables -- even in cases when it would be profitable? Does this make let
> > > slower than var?
> > > 
> > For captured 'let' variables, yes, it will prevent optimizations and be
> > slower. 
> > It probably makes sense to make the TDZ flag value not be empty JSValue
> > at some point so we don't run into this problem. 
> > 
> > Also, I think 'let' will always be slower than 'var' by nature 
> > of the TDZ. It's kind of crappy that this is the case because
> > 'let' is a lot nicer than 'var'. The DFG should eliminate almost all
> > TDZ checks, so for hot enough code, it probably won't be that
> > big of a performance hit.
> 
> Are there any cases where we can rescue the constant closure optimization?
> It's a really important optimization, especially in asm.js, and we'll suffer
> huge losses if people mass switch from var to let.
I was wrong. We will be able to do this optimization always.
The solution is just to initialize to the empty TDZ value when allocating
JSLexicalEnvironment instead of in byte code.

> 
> > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:594
> > > > +RegisterID* BytecodeGenerator::newLexicalVariable()
> > > 
> > > I would call this "newLet" or "newBlockScopeVariable". "Lexical" is kind of
> > > a nebulous term.
> > I like "newBlockScopeVariable".
> > 
> > > 
> > > > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:710
> > > > +        Vector<SymbolTableStackEntry> m_symbolTableStack;
> > > 
> > > What's the plan for making the stack of symbol tables available to the
> > > debugger?
> > > 
> > > I don't think the debugger can work with this design: I think the stack
> > > needs to be a run-time data structure, not a compile-time data structure.
> > Doesn't JSScope act as a defacto stack?
> 
> JSScope does act as a stack. But you aren't emitting a JSScope for every let
> scope. You're only emitting a JSScope if the let scope is captured. There's
> the rub. You can either emit a JSScope for every let scope or find a way to
> represent the compile-time-only scope stack in a way that the debugger can
> consume.
Right, that makes sense. I didn't think of this.

After looking at the debugger code in BytecodeGenerator, it looks like
we will capture all "var"s when the debugger is enabled. Maybe a good 
starting point for block scoped variables is also to capture everything, 
and then we can try to further optimize in a later patch. What do you think?

[Geoffrey Garen]

> > Are there any cases where we can rescue the constant closure optimization?
> > It's a really important optimization, especially in asm.js, and we'll suffer
> > huge losses if people mass switch from var to let.
> I was wrong. We will be able to do this optimization always.
> The solution is just to initialize to the empty TDZ value when allocating
> JSLexicalEnvironment instead of in byte code.

Sounds good.

> > JSScope does act as a stack. But you aren't emitting a JSScope for every let
> > scope. You're only emitting a JSScope if the let scope is captured. There's
> > the rub. You can either emit a JSScope for every let scope or find a way to
> > represent the compile-time-only scope stack in a way that the debugger can
> > consume.
> Right, that makes sense. I didn't think of this.
> 
> After looking at the debugger code in BytecodeGenerator, it looks like
> we will capture all "var"s when the debugger is enabled. Maybe a good 
> starting point for block scoped variables is also to capture everything, 
> and then we can try to further optimize in a later patch. What do you think?

I think this is OK. The penalty is worse, since you'll have an allocation per loop invocation instead of an allocation per function invocation. Still, it will work.

[Saam Barati]

> I think this is OK. The penalty is worse, since you'll have an allocation
> per loop invocation instead of an allocation per function invocation. Still,
> it will work.
Right. We should find a way to speed things up.

[Saam Barati]

Created attachment 253794 [details]
patch

This is almost done. I just want to write tests for allocation sinking with 'let' variables (TDZ and TDZ during OSR exit, etc).
I also need to make it such that all variables are captured when the debugger is enabled (this should be only a few lines) and make
sure that this works.

Comments/recommendations on patch are appreciated.

[WebKit Commit Bot]

Attachment 253794 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:774:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 69 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

*** Bug 141885 has been marked as a duplicate of this bug. ***

[Saam Barati]

(In reply to comment #50)
> Created attachment 253794 [details]
> patch
> 
> This is almost done. I just want to write tests for allocation sinking with
> 'let' variables (TDZ and TDZ during OSR exit, etc).
> I also need to make it such that all variables are captured when the
> debugger is enabled (this should be only a few lines) and make
> sure that this works.
> 
> Comments/recommendations on patch are appreciated.

On the topic of the debugger, there are some decisions to make regarding
how we land this. We can land this patch as is (or close to it), which will 
break the debugger if paused on a breakpoint inside a lexical scope that has values 
that are still the empty TDZ value. There are a few places where we assert
against something being the TDZ empty value (JSValue()), and the debugger
will crash because of this. For example, PropertySlot will assert against
the empty value being set inside ::setValue. But, given that this can be done
by the debugger when calling JSLexicalEnvironment::SymbolTableGet when
paused on a breakpoint in a lexical scope, having the TDZ value here makes perfect sense.

Are these assertions worth removing now that it's perfectly valid for an
activation to have the empty JSValue? Or worth removing in a separate patch? 
Do these assertions help find real bugs or are they there more as a documentation 
measure? I guess another solution could be to come up with another value for the 
TDZ empty value, but this doesn't sound great to me.

We will also need some new UI in the inspector, too, for showing that a value is the empty
TDZ value.

[Saam Barati]

Created attachment 255536 [details]
patch

[WebKit Commit Bot]

Attachment 255536 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/InspectorTimelineAgent.cpp:715:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:791:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 4 in 80 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 255614 [details]
patch

Same patch now with a ChangeLog.

Perf results looked good when I checked last night on an older build.
I'll upload again once I'm done compiling.

[WebKit Commit Bot]

Attachment 255614 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:791:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 3 in 81 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 255622 [details]
perf results

Looks neutral, as expected.

[Saam Barati]

(In reply to comment #58)
> Created attachment 255622 [details]
> perf results
> 
> Looks neutral, as expected.

Scratch that, looks like there are a couple slow downs.
I'll re-run to see if this is real.

[Saam Barati]

(In reply to comment #59)
> (In reply to comment #58)
> > Created attachment 255622 [details]
> > perf results
> > 
> > Looks neutral, as expected.
> 
> Scratch that, looks like there are a couple slow downs.
> I'll re-run to see if this is real.

I'm getting a somewhat consistent 3-5% slowdown on Octane's jQuery benchmark.
I'll see what I can do to speed up the parser.

[Saam Barati]

Created attachment 255804 [details]
patch

[WebKit Commit Bot]

Attachment 255804 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:152:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:861:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:872:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:936:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 6 in 81 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 255804 [details]
patch

Attachment 255804 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6614096631824384

New failing tests:
js/dom/reserved-words-as-property.html
js/keywords-and-reserved_words.html

[Build Bot]

Created attachment 255806 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 255804 [details]
patch

Attachment 255804 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6704402614190080

New failing tests:
js/dom/reserved-words-as-property.html
js/keywords-and-reserved_words.html

[Build Bot]

Created attachment 255807 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Saam Barati]

Created attachment 255830 [details]
patch

Lets see if EWS likes this patch better.

[WebKit Commit Bot]

Attachment 255830 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:152:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:861:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:872:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:936:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 6 in 81 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 255830 [details]
patch

Attachment 255830 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4732507966668800

New failing tests:
js/dom/reserved-words-as-property.html
js/keywords-and-reserved_words.html

[Build Bot]

Created attachment 255834 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 255830 [details]
patch

Attachment 255830 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5922911839846400

New failing tests:
js/dom/reserved-words-as-property.html
js/keywords-and-reserved_words.html

[Build Bot]

Created attachment 255835 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Saam Barati]

Created attachment 255871 [details]
patch

[WebKit Commit Bot]

Attachment 255871 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:152:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:861:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:872:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 81 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 256033 [details]
patch

Added a Parser environment data structure to represent a parse/bytecode compile time variable environment

[Saam Barati]

*** Bug 144978 has been marked as a duplicate of this bug. ***

[Geoffrey Garen]

> ERROR: Source/JavaScriptCore/ChangeLog:152:  Line contains tab character. 
> [whitespace/tab] [5]

Please fix.

[WebKit Commit Bot]

Attachment 256033 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/VariableEnvironment.cpp:31:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:860:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:868:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ChangeLog:152:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/VariableEnvironment.h:61:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/JavaScriptCore/parser/VariableEnvironment.h:69:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/JavaScriptCore/runtime/JSGlobalObject.h:41:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 9 in 86 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 256036 [details]
patch

fixed needed style issues.

[WebKit Commit Bot]

Attachment 256036 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:860:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:868:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 4 in 86 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 256033 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256033&action=review

You should hold off on landing this until the branch point.

You should turn on let in sloppy mode.

You should prohibit, as a syntax error, let inside a one-line statement that is not a block statement (as in if, while, for, etc.), since that is specified, and not prohibiting it produces some surprising behaviors that invalidate the optimization I suggested.

> Source/JavaScriptCore/ChangeLog:22
> +        3) op_get_from_scope
> +        This opcode is now modified to perform a TDZ check on its result.
> +        4) op_get_from_scope_no_tdz
> +        This opcode is the same as op_get_from_scope except we emit it
> +        when we have the static guarantee that we will not need a TDZ check.
> +        Various changes were made to ensure code sharing between these two opcodes.

Two issues here:

(1) I think it's better for an opcode to say what it does rather than what it doesn't do. So, I think it would be better to have op_get_from_scope and op_get_from_scope_tdz, with the latter performing the TDZ check, and the default performing no check.

(2) Why did you decide on a special fused opcode, which reads a value and does a TDZ check, rather than op_get_from_scope followed by op_check_tdz? In general, I think it is best to reuse an existing opcode like op_check_tdz, because that helps to ensure that any TDZ optimizations we do apply in all cases. But perhaps there is a good reason for a fused opcode in this case.

> Source/JavaScriptCore/ChangeLog:356
> +        is appropriating the Scope struct to also also model a lexical 

also also

> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:126
> +    case op_push_lexical_scope:

(1) How does op_push_lexical_scope differ from op_create_lexical_environment? Both seem to allocate a JSLexicalEnvironment and push it on the scope stack.

(2) Either you should call this op_push_lexical_environment (and op_pop_lexical_environment), or you should rename op_create_lexical_environment to op_create_lexical_scope and JSLexicalEnvironment to JSLexicalScope.

> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:46
> -static_assert(sizeof(UnlinkedFunctionExecutable) <= 128, "UnlinkedFunctionExecutable should fit in a 128-byte cell.");
> +static_assert(sizeof(UnlinkedFunctionExecutable) <= 256, "UnlinkedFunctionExecutable should fit in a 256-byte cell.");

It was a memory win to fix unlinked executables in 128 byte cells. Is there any way to get this memory back?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1896
> +bool BytecodeGenerator::needsTDZCheck(const Variable& variable)

I think this mechanism should be much more aggressive. Namely:

In NodesCodegen.cpp, the code generation for the "let x" node should tell the BytecodeGenerator, after it generates code for the RHS, that x no longer requires TDZ checks. The BytecodeGenerator should then remove x from its hash of variables requiring TDZ checks. In other words, we should only do a TDZ check in the case where x appears in the right hand side of its own definition, or prior to its own definition.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1898
> +    // FIXME: When allowing 'let' in sloppy mode, we need to consider other factors here such as 'eval'.

Remove this comment because it is incorrect according to 18.2.1.1, which says that each eval gets a unique NewDeclarativeEnvironment.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2088
>  RegisterID* EmptyVarExpression::emitBytecode(BytecodeGenerator& generator, RegisterID*)

I'm confused by the use of Var to mean "Var or Let", since they otherwise have different semantics. Can we have two different AST nodes?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2254
> +    // FIXME: We can optimize TDZ checks here because once a for loop's
> +    // header has executed, it is guaranteed that inside the body of the
> +    // loop that the loop header's lexical variables will no longer
> +    // be under TDZ.
> +    RegisterID* forLoopSymbolTable;

I think my suggestion above covers this FIXME.

> Source/JavaScriptCore/parser/ASTBuilder.h:528
> -    ExpressionNode* createEmptyVarExpression(const JSTokenLocation& location, const Identifier& identifier)
> +    StatementNode* createLetStatement(const JSTokenLocation& location, ExpressionNode* expr, int start, int end)
>      {
> -        return new (m_parserArena) EmptyVarExpression(location, identifier);
> +        // FIXME: better name for 'VarStatementNode'.
> +        return createVarStatement(location, expr, start, end);
> +    }

It's pretty weird for this mutinous function to hear your request for a let statement and then product a var statement. Maybe we should rename to DeclarationStatment or LetOrVarStatement? Or maybe just keep calling it Var or Variable -- but we should use a term consistently.

> Source/JavaScriptCore/parser/Nodes.h:206
> +    class LexicalNode {

Let's call this EnvironmentNode or VariableEnvironmentNode. Lexical is a problematic term in a parsing context because it means "relating to words", which is everything.

> Source/JavaScriptCore/parser/Parser.cpp:860
> +    auto gatherLexicalVariablesIfNecessary = [&] () {

Please omit the empty parameter list.

> Source/JavaScriptCore/parser/Parser.cpp:868
> +    auto popLexicalScopeIfNecessary = [&] () {

Please omit the empty parameter list.

> Source/JavaScriptCore/parser/VariableEnvironment.h:53
> +class VariableEnvironment {

I think this is a pretty neat class. But it hurts my brain that we treat let and var so differently when they are not different in this respect. It would be nice to use this class everywhere.

[Filip Pizlo]

(In reply to comment #81)
> Comment on attachment 256033 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256033&action=review
> 
> You should hold off on landing this until the branch point.
> 
> You should turn on let in sloppy mode.
> 
> You should prohibit, as a syntax error, let inside a one-line statement that
> is not a block statement (as in if, while, for, etc.), since that is
> specified, and not prohibiting it produces some surprising behaviors that
> invalidate the optimization I suggested.
> 
> > Source/JavaScriptCore/ChangeLog:22
> > +        3) op_get_from_scope
> > +        This opcode is now modified to perform a TDZ check on its result.
> > +        4) op_get_from_scope_no_tdz
> > +        This opcode is the same as op_get_from_scope except we emit it
> > +        when we have the static guarantee that we will not need a TDZ check.
> > +        Various changes were made to ensure code sharing between these two opcodes.
> 
> Two issues here:
> 
> (1) I think it's better for an opcode to say what it does rather than what
> it doesn't do. So, I think it would be better to have op_get_from_scope and
> op_get_from_scope_tdz, with the latter performing the TDZ check, and the
> default performing no check.

The no_tdz opcode may still do a tdz check on the slow path.

> 
> (2) Why did you decide on a special fused opcode, which reads a value and
> does a TDZ check, rather than op_get_from_scope followed by op_check_tdz? In
> general, I think it is best to reuse an existing opcode like op_check_tdz,
> because that helps to ensure that any TDZ optimizations we do apply in all
> cases. But perhaps there is a good reason for a fused opcode in this case.

Loading from a scope might do a tdz check on the slow path.

The idea that I suggested was:

- Pretend that getting from a scope always does a TDZ check, so that the slow paths in JSScope and friends can just do it. For non-let variables, it's just an extra branch-test-zero on the slow path, which is free.  The benefit is that the code has fewer special conditions.

- Have an optimized form of get_from_scope that means that you don't *have* to do the TDZ check, so that the fast path for loading non-let variables is fast.

Having a separate opcode would mean that the slow paths would have to return the TDZ empty value.  Then we'd have to change a bunch of stuff to allow for the possibility of JSValue() being on the stack and passed around throughout the runtime.  For example, PropertySlot would need to allow JSValue(), which it currently does not do.  The approach used here avoids this entirely by mandating that the TDZ check happens immediately.

> 
> > Source/JavaScriptCore/ChangeLog:356
> > +        is appropriating the Scope struct to also also model a lexical 
> 
> also also
> 
> > Source/JavaScriptCore/bytecode/BytecodeUseDef.h:126
> > +    case op_push_lexical_scope:
> 
> (1) How does op_push_lexical_scope differ from
> op_create_lexical_environment? Both seem to allocate a JSLexicalEnvironment
> and push it on the scope stack.
> 
> (2) Either you should call this op_push_lexical_environment (and
> op_pop_lexical_environment), or you should rename
> op_create_lexical_environment to op_create_lexical_scope and
> JSLexicalEnvironment to JSLexicalScope.
> 
> > Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:46
> > -static_assert(sizeof(UnlinkedFunctionExecutable) <= 128, "UnlinkedFunctionExecutable should fit in a 128-byte cell.");
> > +static_assert(sizeof(UnlinkedFunctionExecutable) <= 256, "UnlinkedFunctionExecutable should fit in a 256-byte cell.");
> 
> It was a memory win to fix unlinked executables in 128 byte cells. Is there
> any way to get this memory back?
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1896
> > +bool BytecodeGenerator::needsTDZCheck(const Variable& variable)
> 
> I think this mechanism should be much more aggressive. Namely:
> 
> In NodesCodegen.cpp, the code generation for the "let x" node should tell
> the BytecodeGenerator, after it generates code for the RHS, that x no longer
> requires TDZ checks. The BytecodeGenerator should then remove x from its
> hash of variables requiring TDZ checks. In other words, we should only do a
> TDZ check in the case where x appears in the right hand side of its own
> definition, or prior to its own definition.
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1898
> > +    // FIXME: When allowing 'let' in sloppy mode, we need to consider other factors here such as 'eval'.
> 
> Remove this comment because it is incorrect according to 18.2.1.1, which
> says that each eval gets a unique NewDeclarativeEnvironment.
> 
> > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2088
> >  RegisterID* EmptyVarExpression::emitBytecode(BytecodeGenerator& generator, RegisterID*)
> 
> I'm confused by the use of Var to mean "Var or Let", since they otherwise
> have different semantics. Can we have two different AST nodes?
> 
> > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2254
> > +    // FIXME: We can optimize TDZ checks here because once a for loop's
> > +    // header has executed, it is guaranteed that inside the body of the
> > +    // loop that the loop header's lexical variables will no longer
> > +    // be under TDZ.
> > +    RegisterID* forLoopSymbolTable;
> 
> I think my suggestion above covers this FIXME.
> 
> > Source/JavaScriptCore/parser/ASTBuilder.h:528
> > -    ExpressionNode* createEmptyVarExpression(const JSTokenLocation& location, const Identifier& identifier)
> > +    StatementNode* createLetStatement(const JSTokenLocation& location, ExpressionNode* expr, int start, int end)
> >      {
> > -        return new (m_parserArena) EmptyVarExpression(location, identifier);
> > +        // FIXME: better name for 'VarStatementNode'.
> > +        return createVarStatement(location, expr, start, end);
> > +    }
> 
> It's pretty weird for this mutinous function to hear your request for a let
> statement and then product a var statement. Maybe we should rename to
> DeclarationStatment or LetOrVarStatement? Or maybe just keep calling it Var
> or Variable -- but we should use a term consistently.
> 
> > Source/JavaScriptCore/parser/Nodes.h:206
> > +    class LexicalNode {
> 
> Let's call this EnvironmentNode or VariableEnvironmentNode. Lexical is a
> problematic term in a parsing context because it means "relating to words",
> which is everything.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:860
> > +    auto gatherLexicalVariablesIfNecessary = [&] () {
> 
> Please omit the empty parameter list.
> 
> > Source/JavaScriptCore/parser/Parser.cpp:868
> > +    auto popLexicalScopeIfNecessary = [&] () {
> 
> Please omit the empty parameter list.
> 
> > Source/JavaScriptCore/parser/VariableEnvironment.h:53
> > +class VariableEnvironment {
> 
> I think this is a pretty neat class. But it hurts my brain that we treat let
> and var so differently when they are not different in this respect. It would
> be nice to use this class everywhere.

[Saam Barati]

> I think this mechanism should be much more aggressive. Namely:
> 
> In NodesCodegen.cpp, the code generation for the "let x" node should tell
> the BytecodeGenerator, after it generates code for the RHS, that x no longer
> requires TDZ checks. The BytecodeGenerator should then remove x from its
> hash of variables requiring TDZ checks. In other words, we should only do a
> TDZ check in the case where x appears in the right hand side of its own
> definition, or prior to its own definition.
> 
I just realized one problem with this. "switch" statements
will not play well with this plan. Maybe we can implement this
optimization and just prevent lexical variables of a "switch"
statement from being optimized.

I'm wondering if this optimization is worth doing in its own
patch after this lands. Regardless, I'll start working on it.

[Saam Barati]

Comment on attachment 256033 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256033&action=review

>> Source/JavaScriptCore/bytecode/BytecodeUseDef.h:126
>> +    case op_push_lexical_scope:
> 
> (1) How does op_push_lexical_scope differ from op_create_lexical_environment? Both seem to allocate a JSLexicalEnvironment and push it on the scope stack.
> 
> (2) Either you should call this op_push_lexical_environment (and op_pop_lexical_environment), or you should rename op_create_lexical_environment to op_create_lexical_scope and JSLexicalEnvironment to JSLexicalScope.

They differ in a couple ways:
push_lexical_scope takes as an opcode argument the SymbolTable of
the scope it creates. Also, push_lexical_scope will allocate JSLexicalEnvironment
with JSValue() in all variable slots where create_lexical_environment
will allocate JSLexicalEnvironment with jsUndefined() in all slots.

I will rename this to op_(push/pop)_lexical_environment.

>> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:46
>> +static_assert(sizeof(UnlinkedFunctionExecutable) <= 256, "UnlinkedFunctionExecutable should fit in a 256-byte cell.");
> 
> It was a memory win to fix unlinked executables in 128 byte cells. Is there any way to get this memory back?

I'll see if there are fields we can get rid of or if it's worth creating a rare data structure.

>> Source/JavaScriptCore/parser/VariableEnvironment.h:53
>> +class VariableEnvironment {
> 
> I think this is a pretty neat class. But it hurts my brain that we treat let and var so differently when they are not different in this respect. It would be nice to use this class everywhere.

I will use this everywhere.

[Saam Barati]

(In reply to comment #83)
> > I think this mechanism should be much more aggressive. Namely:
> > 
> > In NodesCodegen.cpp, the code generation for the "let x" node should tell
> > the BytecodeGenerator, after it generates code for the RHS, that x no longer
> > requires TDZ checks. The BytecodeGenerator should then remove x from its
> > hash of variables requiring TDZ checks. In other words, we should only do a
> > TDZ check in the case where x appears in the right hand side of its own
> > definition, or prior to its own definition.
> > 
> I just realized one problem with this. "switch" statements
> will not play well with this plan. Maybe we can implement this
> optimization and just prevent lexical variables of a "switch"
> statement from being optimized.
Add try/catch to this list as well.

[Saam Barati]

(In reply to comment #85)
> (In reply to comment #83)
> > > I think this mechanism should be much more aggressive. Namely:
> > > 
> > > In NodesCodegen.cpp, the code generation for the "let x" node should tell
> > > the BytecodeGenerator, after it generates code for the RHS, that x no longer
> > > requires TDZ checks. The BytecodeGenerator should then remove x from its
> > > hash of variables requiring TDZ checks. In other words, we should only do a
> > > TDZ check in the case where x appears in the right hand side of its own
> > > definition, or prior to its own definition.
> > > 
> > I just realized one problem with this. "switch" statements
> > will not play well with this plan. Maybe we can implement this
> > optimization and just prevent lexical variables of a "switch"
> > statement from being optimized.
> Add try/catch to this list as well.
Never mind, these should optimize just fine.

[Saam Barati]

(In reply to comment #81)

> (1) I think it's better for an opcode to say what it does rather than what
> it doesn't do. So, I think it would be better to have op_get_from_scope and
> op_get_from_scope_tdz, with the latter performing the TDZ check, and the
> default performing no check.

When Fil and I spoke about this, I originally argued this same point.
Fil convinced me that because get_from_scope is always safe to execute
it should contain the shorter name. Because get_from_scope_no_tdz is
the specialized op code, it should be longer and explicit in its intentions.

[Saam Barati]

Created attachment 256226 [details]
patch

Almost there. I re-read the ES6 grammar and there are a few places where things need to be changed
now that we're allowing this in sloppy mode.
Basically, in sloppy mode, we need to allow:
"var let = 20;"
but not:
"let let = 20"
"const let = 20"
There are a few other variations as well.

Stay classy, ES.

[WebKit Commit Bot]

Attachment 256226 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1360:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:418:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 4 in 83 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 256226 [details]
patch

Attachment 256226 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5637634474901504

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 256232 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 256226 [details]
patch

Attachment 256226 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5662538037460992

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 256233 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Saam Barati]

Created attachment 256257 [details]
patch

[WebKit Commit Bot]

Attachment 256257 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:518:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1310:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1360:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2468:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2909:  Missing space before {  [whitespace/braces] [5]
Total errors found: 7 in 84 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 256257 [details]
patch

Attachment 256257 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5139350622830592

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 256264 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 256257 [details]
patch

Attachment 256257 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5806063764897792

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 256267 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Geoffrey Garen]

Looks like this still needs some bug fixing.

[Geoffrey Garen]

> I just realized one problem with this. "switch" statements
> will not play well with this plan.

Can you provide an example? (This would be a good test case to add, too.)

[Geoffrey Garen]

> > I just realized one problem with this. "switch" statements
> > will not play well with this plan.
> 
> Can you provide an example? (This would be a good test case to add, too.)

Oh, I see your updated comment now, saying that the optimization will work. Would still be good to include test cases that match the thing that you thought would go wrong.

[Geoffrey Garen]

> > (1) How does op_push_lexical_scope differ from op_create_lexical_environment? Both seem to allocate a JSLexicalEnvironment and push it on the scope stack.

> They differ in a couple ways:
> push_lexical_scope takes as an opcode argument the SymbolTable of
> the scope it creates. Also, push_lexical_scope will allocate
> JSLexicalEnvironment
> with JSValue() in all variable slots where create_lexical_environment
> will allocate JSLexicalEnvironment with jsUndefined() in all slots.

These differences don't seem essential, and I think you should be able to remove op_create_lexical_environment and replace it with op_push_lexical_scope, with two small changes:

(1) When we need to push a function scope, put the CodeBlock's symbol table into the constant pool and use that as the symbol table operand for op_push_lexical_scope.

(2) Add an operand to op_push_lexical_scope that specifies the fill value to use.

I think this would be a nice simplification without any noticeable performance penalty.

[Geoffrey Garen]

> Having a separate opcode would mean that the slow paths would have to return
> the TDZ empty value.  Then we'd have to change a bunch of stuff to allow for
> the possibility of JSValue() being on the stack and passed around throughout
> the runtime.  For example, PropertySlot would need to allow JSValue(), which
> it currently does not do.  The approach used here avoids this entirely by
> mandating that the TDZ check happens immediately.

I think we face the TDZ issue inevitably. We store JSValue() on the stack a lot -- for let variables and for 'this' in class scope. We initialize the slots in a JSLexicalEnvironment to TDZ. The debugger can resolve those TDZ values. The GC can see them too.

So, our options are:

(1) Do the TDZ check with op_check_tdz: Add to DebuggerScope::getOwnPropertySlot a check for TDZ, and remove from PropertySlot the ASSERT against TDZ.

(2) Do the TDZ check by default in op_get_from_scope: Add a new opcode that does or does not do the TDZ check, add to JSLexicalEnvironment::::symbolTableGet special handling to throw on TDZ, and add to DebuggerScope::getOwnPropertySlot special handling to catch the throw and do something else with it.

In the end, (2) seems like more special handling to me, and it doesn't fully remove TDZ from the runtime. Also, (1) does not leak TDZ anywhere other than PropertySlot. So, I prefer (1).

[Geoffrey Garen]

EvalCodeCache::tryGet is broken in a world of let because it assumes that the presence of a variable object means a predictable scope chain. But in a world of let, where lets are stored on variable objects, two evals might be in different scopes if one was in a let scope and one was not.

[Geoffrey Garen]

If every resolve inside eval is dynamic, any caching is valid, and the variable object check is not necessary. If no resolves are dynamic, everything is broken. See this example:

function a() { var x = 1; return eval("x"); }
function b() { return eval("x"); }

[Geoffrey Garen]

To fix the bug where eval cannot find the variable object, since there may be a few let scopes in the way:

I suggest passing through op_call_eval the relative scope depth inside the function. It is easy for BytecodeGenerator to keep this count, and I think it already does so.

[Geoffrey Garen]

Also: All uses of isVariableObject() are wrong in the world of let, and you should fix them.

[Saam Barati]

(In reply to comment #103)
> > > (1) How does op_push_lexical_scope differ from op_create_lexical_environment? Both seem to allocate a JSLexicalEnvironment and push it on the scope stack.
> 
> > They differ in a couple ways:
> > push_lexical_scope takes as an opcode argument the SymbolTable of
> > the scope it creates. Also, push_lexical_scope will allocate
> > JSLexicalEnvironment
> > with JSValue() in all variable slots where create_lexical_environment
> > will allocate JSLexicalEnvironment with jsUndefined() in all slots.
> 
> These differences don't seem essential, and I think you should be able to
> remove op_create_lexical_environment and replace it with
> op_push_lexical_scope, with two small changes:
> 
> (1) When we need to push a function scope, put the CodeBlock's symbol table
> into the constant pool and use that as the symbol table operand for
> op_push_lexical_scope.
> 
> (2) Add an operand to op_push_lexical_scope that specifies the fill value to
> use.
> 
> I think this would be a nice simplification without any noticeable
> performance penalty.

My new patch will do this.

[Saam Barati]

(In reply to comment #104)
> > Having a separate opcode would mean that the slow paths would have to return
> > the TDZ empty value.  Then we'd have to change a bunch of stuff to allow for
> > the possibility of JSValue() being on the stack and passed around throughout
> > the runtime.  For example, PropertySlot would need to allow JSValue(), which
> > it currently does not do.  The approach used here avoids this entirely by
> > mandating that the TDZ check happens immediately.
> 
> I think we face the TDZ issue inevitably. We store JSValue() on the stack a
> lot -- for let variables and for 'this' in class scope. We initialize the
> slots in a JSLexicalEnvironment to TDZ. The debugger can resolve those TDZ
> values. The GC can see them too.
> 
> So, our options are:
> 
> (1) Do the TDZ check with op_check_tdz: Add to
> DebuggerScope::getOwnPropertySlot a check for TDZ, and remove from
> PropertySlot the ASSERT against TDZ.
> 
> (2) Do the TDZ check by default in op_get_from_scope: Add a new opcode that
> does or does not do the TDZ check, add to
> JSLexicalEnvironment::::symbolTableGet special handling to throw on TDZ, and
> add to DebuggerScope::getOwnPropertySlot special handling to catch the throw
> and do something else with it.
> 
> In the end, (2) seems like more special handling to me, and it doesn't fully
> remove TDZ from the runtime. Also, (1) does not leak TDZ anywhere other than
> PropertySlot. So, I prefer (1).

I'm going with (1). Also, I am making DebuggerScope aware that a property
slot may have the TDZ value. If the DebuggerScope sees TDZ value, it will
set the value to undefined (for now, at least). This prevents TDZ value
from leaking out into weird places in the runtime.

[Saam Barati]

(In reply to comment #107)
> To fix the bug where eval cannot find the variable object, since there may
> be a few let scopes in the way:
> 
> I suggest passing through op_call_eval the relative scope depth inside the
> function. It is easy for BytecodeGenerator to keep this count, and I think
> it already does so.

There is another approach Geoff and I discussed offline. It's the approach
I will use.

Currently, we walk the scope chain to find the proper "var" environment (or
the global object). We will continue to do this and just make sure we walk past
all lexical environments. JSScope will know which environments correspond to lexical
environments because JSLexicalEnvironment's symbol table is aware of it being
a lexical environment.

Also, EvalCodeCache caching is broken in a world of "let". Here is a simple
fix. We will still be able to cache eval as long as the current JSScope
is not a "lexical scope". This is a simple check (see above). Eval's can
be cached as long as the scope they're caching with respect to is not a lexical scope.
If it's a lexical scope, we just won't cache the eval because variable resolution
won't be guaranteed to be the same across cache hits.

[Saam Barati]

Created attachment 256433 [details]
patch

[WebKit Commit Bot]

Attachment 256433 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:521:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1315:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2470:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2911:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 6 in 92 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 256438 [details]
patch

same patch as before but with a comment to clarify one bit of code.

[WebKit Commit Bot]

Attachment 256438 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:523:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1317:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2472:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2913:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 6 in 92 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 256438 [details]
patch

Attachment 256438 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4869173557592064

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 256445 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 256438 [details]
patch

Attachment 256438 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6181646843772928

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 256450 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Saam Barati]

Created attachment 256465 [details]
patch

[WebKit Commit Bot]

Attachment 256465 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:523:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1317:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2474:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2915:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 6 in 92 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 256481 [details]
patch

fix armv7 build.

[WebKit Commit Bot]

Attachment 256481 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:523:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1317:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2474:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2915:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 6 in 92 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

*** Bug 144979 has been marked as a duplicate of this bug. ***

[Filip Pizlo]

Comment on attachment 256481 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256481&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2577
> +void ByteCodeParser::handleTDZCheck(VirtualRegister operand)
> +{
> +    addToGraph(CheckNotEmpty, get(operand));
> +}

You don't need this helper.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3735
> +        case op_push_lexical_environment: {

Why is this called "push" when it appears to just create?  I understand that you use it as part of a push - but the semantics of the instruction itself don't do that.  I think that's very misleading.  Maybe we can keep the original name of the opcode?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3744
> +            addToGraph(Phantom, scope);

You don't need this phantom.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3754
> +        case op_pop_lexical_environment: {
> +            Node* currentScope = get(VirtualRegister(currentInstruction[1].u.operand));
> +            Node* newScope = addToGraph(SkipScope, currentScope);
> +            set(VirtualRegister(currentInstruction[1].u.operand), newScope);
> +            addToGraph(Phantom, currentScope);
> +            NEXT_OPCODE(op_pop_lexical_environment);
> +        }

Similar comment.  This doesn't pop anything.  I would call this op_get_scope_parent or something like this.

[Filip Pizlo]

Comment on attachment 256481 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256481&action=review

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1887
> +    Vector<SymbolTable*> symbolTableStack;

Why is this necessary?  Wouldn't it be better if each SymbolTable had a pointer to the SymbolTable that was its parent?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2020
> +            symbolTableStack.append(linkedTable);

This is scary.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2025
> +            symbolTableStack.removeLast();

This is scary.

[Saam Barati]

Created attachment 256658 [details]
patch

Took into consideration Fil's comments.

This is what we decided on:
- remove the SymbolTableStack from CodeBlock
- op_put_to_scope knows which SymbolTable it puts into for LocalClosureVar. This allows it to handle
the logic of ensuring the linked SymbolTable is cloned. It now takes as an argument the index into the
constant pool for the SymbolTable. This index is only used to communicate from unlinked code to the linked
code generator in CodeBlock. This index isn't present in the linked code.
- op_put_to_scope for non LocalClosureVar, op_get_from_scope, op_resolve_scope, op_profile_type,
all take as an argument the local scope depth that JSScope::abstractResolve will use when
doing resolution in the function's parent scope/GlobalObject. This, too, is only
used to communicate from unlinked code to the linked code generator. It is not present
in the linked code.

[WebKit Commit Bot]

Attachment 256658 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:525:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1321:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2481:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2944:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 6 in 91 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 256658 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=256658&action=review

> Source/JavaScriptCore/bytecode/EvalCodeCache.h:79
> +            // If eval() is called and it has access to a lexical scope, we can't soundly cache it.

Why?

> Source/JavaScriptCore/dfg/DFGOperations.cpp:765
> -JSCell* JIT_OPERATION operationCreateActivationDirect(ExecState* exec, Structure* structure, JSScope* scope, SymbolTable* table)
> +JSCell* JIT_OPERATION operationPushLexicalEnvironmentDirect(ExecState* exec, Structure* structure, JSScope* scope, SymbolTable* table, EncodedJSValue initialValueEncoded)

Seems like this should be called CreateActivation not PushLexicalEnvironment.

> Source/JavaScriptCore/jit/JITOperations.cpp:1446
> +EncodedJSValue JIT_OPERATION operationPushLexicalEnvironment(ExecState* exec, Instruction* bytecodePC)

This also seems like it's creating, not pushing.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1294
> +LLINT_SLOW_PATH_DECL(slow_path_create_lexical_environment)

You could make this a CommonSlowPath and have the JIT call it, which would obviate the need for a Create operation in JITOperations.

[Filip Pizlo]

Are the perf results current?  It looks like they are super noisy.  Try running with "--outer 10" on a machine that is disconnected from the network and all apps other than Terminal closed.

If the 4% Kraken slow-down is real, then we should investigate.

[Saam Barati]

VMs tested:
"original" at /Volumes/Data/WK/c/OpenSource/WebKitBuild/Release/jsc (r186799)
"letScoping" at /Volumes/Data/WK/b/OpenSource/WebKitBuild/Release/jsc (r186799)

Collected 10 samples per benchmark/VM, with 10 VM invocations per benchmark. Emitted a call to gc() between sample measurements.
Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to get microsecond-level
timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds.

                                                         original                 letScoping                                    
SunSpider:
   3d-cube                                            4.0159+-0.0577     ?      4.2190+-0.3093        ? might be 1.0506x slower
   3d-morph                                           4.8977+-0.1150            4.8823+-0.0690        
   3d-raytrace                                        4.9098+-0.1972            4.8179+-0.0475          might be 1.0191x faster
   access-binary-trees                                1.9325+-0.0781     ?      2.1198+-0.3541        ? might be 1.0969x slower
   access-fannkuch                                    5.0022+-0.1482            4.9886+-0.1248        
   access-nbody                                       2.4516+-0.2017            2.3045+-0.0802          might be 1.0638x faster
   access-nsieve                                      3.1244+-0.3341            2.9183+-0.1107          might be 1.0706x faster
   bitops-3bit-bits-in-byte                           1.4257+-0.1393            1.3757+-0.0450          might be 1.0363x faster
   bitops-bits-in-byte                                3.0613+-0.0654     ?      3.0716+-0.0304        ?
   bitops-bitwise-and                                 1.9411+-0.1667            1.8812+-0.0344          might be 1.0318x faster
   bitops-nsieve-bits                                 2.7336+-0.0524            2.7298+-0.0291        
   controlflow-recursive                              1.8773+-0.0830            1.8380+-0.0448          might be 1.0214x faster
   crypto-aes                                         3.5121+-0.0578     ?      3.5279+-0.0545        ?
   crypto-md5                                         2.3002+-0.0428     ?      2.3334+-0.1656        ? might be 1.0145x slower
   crypto-sha1                                        2.3930+-0.4118            2.2831+-0.1255          might be 1.0482x faster
   date-format-tofte                                  6.3334+-0.1923            6.2645+-0.1338          might be 1.0110x faster
   date-format-xparb                                  4.5093+-0.2191     ?      4.6442+-0.3039        ? might be 1.0299x slower
   math-cordic                                        2.6138+-0.1067     ?      2.6762+-0.1277        ? might be 1.0239x slower
   math-partial-sums                                  4.1994+-0.0769     ?      4.2885+-0.1831        ? might be 1.0212x slower
   math-spectral-norm                                 1.6712+-0.0182     ?      1.7141+-0.0491        ? might be 1.0256x slower
   regexp-dna                                         6.2940+-0.4083     ?      6.3836+-0.4156        ? might be 1.0142x slower
   string-base64                                      4.0454+-0.0891     ?      4.2605+-0.2156        ? might be 1.0532x slower
   string-fasta                                       5.5430+-0.1167     ?      5.5809+-0.1800        ?
   string-tagcloud                                    7.7407+-0.2597     ?      7.8118+-0.3622        ?
   string-unpack-code                                18.9900+-0.4418           18.6973+-0.3008          might be 1.0157x faster
   string-validate-input                              4.2958+-0.0792            4.2911+-0.1088        

   <arithmetic>                                       4.3006+-0.0386     ?      4.3040+-0.0366        ? might be 1.0008x slower

                                                         original                 letScoping                                    
LongSpider:
   3d-cube                                          738.7282+-3.6512     ^    730.8526+-3.6824        ^ definitely 1.0108x faster
   3d-morph                                        1420.9210+-11.6581        1416.9465+-6.4895        
   3d-raytrace                                      578.3348+-2.9188     ^    569.2330+-3.4346        ^ definitely 1.0160x faster
   access-binary-trees                              768.4426+-5.6380          762.8543+-2.5345        
   access-fannkuch                                  259.5024+-1.3962     ?    262.9516+-2.1679        ? might be 1.0133x slower
   access-nbody                                     486.5558+-12.3390         480.9381+-4.2448          might be 1.0117x faster
   access-nsieve                                    345.8087+-6.5173     ?    348.5083+-8.8982        ?
   bitops-3bit-bits-in-byte                          37.6449+-0.2079     ?     37.7987+-0.2946        ?
   bitops-bits-in-byte                               74.9922+-1.0751           73.8683+-3.0445          might be 1.0152x faster
   bitops-nsieve-bits                               383.5160+-2.6591     ?    384.9547+-5.5351        ?
   controlflow-recursive                            406.3625+-3.6106     ?    411.7358+-4.7576        ? might be 1.0132x slower
   crypto-aes                                       548.2188+-2.7169          545.3358+-2.6322        
   crypto-md5                                       440.8382+-2.5010     ?    441.8329+-6.2527        ?
   crypto-sha1                                      592.7944+-7.6630          592.1067+-7.3755        
   date-format-tofte                                468.3020+-6.6446     ?    471.0407+-4.6785        ?
   date-format-xparb                                633.1332+-14.5257    ?    651.0921+-21.6852       ? might be 1.0284x slower
   hash-map                                         142.6665+-2.0926          142.5617+-1.1715        
   math-cordic                                      464.6269+-14.8248         456.8031+-0.6195          might be 1.0171x faster
   math-partial-sums                                389.3148+-3.7419     ?    396.8198+-15.3409       ? might be 1.0193x slower
   math-spectral-norm                               518.3651+-1.4532     ?    520.4903+-3.9860        ?
   string-base64                                    323.7259+-3.4603          322.4968+-3.3377        
   string-fasta                                     341.5338+-1.5650     ^    337.7115+-1.7636        ^ definitely 1.0113x faster
   string-tagcloud                                  168.8878+-1.2352     ?    169.7028+-1.8163        ?

   <geometric>                                      366.6081+-0.7713     ?    366.6319+-1.4210        ? might be 1.0001x slower

                                                         original                 letScoping                                    
V8Spider:
   crypto                                            45.7858+-1.4512     ?     46.1538+-1.9806        ?
   deltablue                                         74.4097+-9.5243     ?     76.0971+-2.3877        ? might be 1.0227x slower
   earley-boyer                                      36.5681+-0.4714     ?     36.6847+-0.2995        ?
   raytrace                                          26.5353+-0.4714     ?     27.3575+-0.6080        ? might be 1.0310x slower
   regexp                                            56.9449+-0.2315     !     58.4718+-0.7703        ! definitely 1.0268x slower
   richards                                          64.7469+-2.0153     ?     65.4901+-1.5723        ? might be 1.0115x slower
   splay                                             31.9310+-0.6496     ?     32.4641+-0.7844        ? might be 1.0167x slower

   <geometric>                                       45.1778+-0.7865     ?     46.0138+-0.5249        ? might be 1.0185x slower

                                                         original                 letScoping                                    
Octane:
   encrypt                                           0.19292+-0.00050          0.19202+-0.00402       
   decrypt                                           3.15013+-0.01924          3.14585+-0.02308       
   deltablue                                x2       0.15408+-0.01914          0.14841+-0.00149         might be 1.0382x faster
   earley                                            0.26403+-0.00187          0.26304+-0.00153       
   boyer                                             3.90183+-0.01900    ?     3.93384+-0.07104       ?
   navier-stokes                            x2       4.67988+-0.01736    ?     4.68843+-0.01871       ?
   raytrace                                 x2       0.96180+-0.02922    ?     0.97855+-0.03761       ? might be 1.0174x slower
   richards                                 x2       0.09593+-0.00113    ?     0.09633+-0.00193       ?
   splay                                    x2       0.33593+-0.03024    ?     0.34173+-0.03884       ? might be 1.0173x slower
   regexp                                   x2      23.38415+-0.33411    ?    23.46863+-0.29074       ?
   pdfjs                                    x2      35.40305+-0.41354    ?    35.60633+-0.21632       ?
   mandreel                                 x2      41.03307+-0.27058    ?    41.44676+-0.38542       ? might be 1.0101x slower
   gbemu                                    x2      32.66442+-1.42978         32.62589+-1.19942       
   closure                                           0.50204+-0.00242    !     0.52727+-0.00356       ! definitely 1.0503x slower
   jquery                                            6.47541+-0.04833    !     6.70940+-0.02624       ! definitely 1.0361x slower
   box2d                                    x2       9.25942+-0.04058          9.25608+-0.08737       
   zlib                                     x2     356.18914+-8.11737    ?   362.77909+-10.03414      ? might be 1.0185x slower
   typescript                               x2     604.20715+-8.37990        601.12012+-8.78456       

   <geometric>                                       5.28037+-0.05362    ?     5.30906+-0.03343       ? might be 1.0054x slower

                                                         original                 letScoping                                    
Kraken:
   ai-astar                                          223.332+-6.630            221.774+-6.799         
   audio-beat-detection                               72.284+-7.227             69.315+-0.723           might be 1.0428x faster
   audio-dft                                         104.827+-2.168            103.178+-1.660           might be 1.0160x faster
   audio-fft                                          59.700+-0.106             58.388+-2.832           might be 1.0225x faster
   audio-oscillator                                   57.937+-0.946      ?      60.756+-5.711         ? might be 1.0487x slower
   imaging-darkroom                                   87.357+-0.766             86.975+-0.811         
   imaging-desaturate                                 49.366+-2.634      ?      51.217+-1.676         ? might be 1.0375x slower
   imaging-gaussian-blur                              79.798+-2.143             78.559+-0.373           might be 1.0158x faster
   json-parse-financial                               35.101+-0.117      ?      35.881+-1.005         ? might be 1.0222x slower
   json-stringify-tinderbox                           25.070+-0.969      ?      26.841+-0.963         ? might be 1.0707x slower
   stanford-crypto-aes                                52.024+-6.679             49.539+-0.248           might be 1.0502x faster
   stanford-crypto-ccm                                36.649+-0.913      ?      40.421+-8.444         ? might be 1.1029x slower
   stanford-crypto-pbkdf2                             92.577+-8.482             91.816+-1.866         
   stanford-crypto-sha256-iterative                   34.328+-0.605      ?      34.545+-0.216         ?

   <arithmetic>                                       72.168+-1.051             72.086+-0.933           might be 1.0011x faster

                                                         original                 letScoping                                    
JSRegress:
   abc-forward-loop-equal                            28.4463+-1.5134           28.2632+-1.1290        
   abc-postfix-backward-loop                         27.6599+-0.6500           27.4428+-0.3283        
   abc-simple-backward-loop                          26.9870+-0.2342     ?     27.8186+-1.1931        ? might be 1.0308x slower
   abc-simple-forward-loop                           27.2891+-0.3168     ?     27.3765+-0.7471        ?
   abc-skippy-loop                                   20.0230+-0.5295     ?     20.1130+-0.5020        ?
   abs-boolean                                        2.2211+-0.0227     ?      2.2624+-0.0363        ? might be 1.0186x slower
   adapt-to-double-divide                            15.7927+-0.7331           15.4327+-0.5013          might be 1.0233x faster
   aliased-arguments-getbyval                         1.0759+-0.0635     ?      1.0995+-0.0408        ? might be 1.0219x slower
   allocate-big-object                                2.4070+-0.2141            2.3795+-0.1385          might be 1.0115x faster
   arguments-named-and-reflective                    10.1244+-0.2761     ?     10.3254+-0.2648        ? might be 1.0199x slower
   arguments-out-of-bounds                            9.3347+-0.2582            9.2607+-0.1949        
   arguments-strict-mode                              8.8354+-0.2276            8.7426+-0.1299          might be 1.0106x faster
   arguments                                          7.9205+-0.1473     ?      7.9624+-0.2320        ?
   arity-mismatch-inlining                            0.7355+-0.0225     ?      0.7431+-0.0226        ? might be 1.0103x slower
   array-access-polymorphic-structure                 5.3918+-0.1487     ?      5.6233+-0.1235        ? might be 1.0429x slower
   array-nonarray-polymorhpic-access                 23.3247+-0.4408     ?     23.4613+-0.7014        ?
   array-prototype-every                             73.9785+-1.2924     ^     70.1930+-0.3847        ^ definitely 1.0539x faster
   array-prototype-forEach                           71.4415+-0.4001     ^     69.2189+-0.2924        ^ definitely 1.0321x faster
   array-prototype-map                               78.1436+-1.1739     ^     76.0625+-0.2549        ^ definitely 1.0274x faster
   array-prototype-reduce                            67.0404+-1.2209     ?     67.0791+-0.6226        ?
   array-prototype-reduceRight                       66.3676+-0.1889     ?     66.3816+-0.5071        ?
   array-prototype-some                              74.0503+-0.8956     ^     70.5151+-0.5562        ^ definitely 1.0501x faster
   array-splice-contiguous                           19.0863+-0.3656     ?     20.4232+-0.9882        ? might be 1.0700x slower
   array-with-double-add                              3.1509+-0.0469     ?      3.1687+-0.0513        ?
   array-with-double-increment                        2.8860+-0.1264            2.7875+-0.0249          might be 1.0353x faster
   array-with-double-mul-add                          3.9411+-0.1549     ?      4.1416+-0.3440        ? might be 1.0509x slower
   array-with-double-sum                              2.9862+-0.0584     ?      3.1312+-0.3337        ? might be 1.0486x slower
   array-with-int32-add-sub                           5.5724+-0.2532            5.3822+-0.0788          might be 1.0353x faster
   array-with-int32-or-double-sum                     2.9700+-0.0171     ?      3.0397+-0.1378        ? might be 1.0235x slower
   ArrayBuffer-DataView-alloc-large-long-lived   
                                                     24.6765+-0.1776           24.5955+-0.3633        
   ArrayBuffer-DataView-alloc-long-lived             11.8786+-0.8035           11.5586+-0.5019          might be 1.0277x faster
   ArrayBuffer-Int32Array-byteOffset                  3.3285+-0.0432     ?      3.3707+-0.0735        ? might be 1.0127x slower
   ArrayBuffer-Int8Array-alloc-large-long-lived   
                                                     24.8322+-0.1525     ?     24.8581+-0.3227        ?
   ArrayBuffer-Int8Array-alloc-long-lived-buffer   
                                                     18.8057+-0.5503     ?     19.1328+-0.9933        ? might be 1.0174x slower
   ArrayBuffer-Int8Array-alloc-long-lived            10.5380+-0.1934     ?     11.0905+-0.8275        ? might be 1.0524x slower
   ArrayBuffer-Int8Array-alloc                        8.9210+-0.1447     ?      8.9900+-0.3446        ?
   asmjs_bool_bug                                    15.8836+-0.6614           15.7339+-0.8811        
   assign-custom-setter-polymorphic                   2.4779+-0.1217            2.3637+-0.0511          might be 1.0483x faster
   assign-custom-setter                               3.2160+-0.0819            3.1093+-0.0374          might be 1.0343x faster
   basic-set                                          7.5134+-0.2851     ?      7.5985+-0.2852        ? might be 1.0113x slower
   big-int-mul                                        3.7793+-0.5440            3.5537+-0.1540          might be 1.0635x faster
   boolean-test                                       2.7334+-0.1521            2.7239+-0.0664        
   branch-fold                                        3.3207+-0.0225     ?      3.4243+-0.1351        ? might be 1.0312x slower
   branch-on-string-as-boolean                       15.7552+-0.8292           15.6000+-0.7005        
   by-val-generic                                     6.7713+-0.3179     ?      7.0435+-0.3510        ? might be 1.0402x slower
   call-spread-apply                                 24.6047+-0.4543           24.2348+-0.4046          might be 1.0153x faster
   call-spread-call                                  20.0615+-0.2452           19.9869+-0.2124        
   captured-assignments                               0.3672+-0.0771            0.3645+-0.0048        
   cast-int-to-double                                 4.7771+-0.0915     ?      4.7947+-0.1055        ?
   cell-argument                                      5.9106+-0.0638     ?      6.0085+-0.1909        ? might be 1.0166x slower
   cfg-simplify                                       2.5615+-0.0411     ?      2.5945+-0.1376        ? might be 1.0129x slower
   chain-getter-access                                7.7480+-0.1692            7.6780+-0.1690        
   cmpeq-obj-to-obj-other                            11.1779+-0.5776     ?     11.5029+-0.2850        ? might be 1.0291x slower
   constant-test                                      4.5622+-0.1203            4.5274+-0.0538        
   create-lots-of-functions                           8.6317+-0.0958     ?      8.6880+-0.3503        ?
   cse-new-array-buffer                               2.1421+-0.1132            2.1360+-0.0742        
   cse-new-array                                      2.2089+-0.0577     ?      2.3034+-0.1428        ? might be 1.0428x slower
   DataView-custom-properties                        29.6747+-1.0564           28.9814+-0.1850          might be 1.0239x faster
   delay-tear-off-arguments-strictmode               11.2640+-0.3368     ?     11.3724+-0.2115        ?
   deltablue-varargs                                141.9996+-2.0635     ?    142.6436+-1.3495        ?
   destructuring-arguments                          151.3963+-1.2323     ?    155.8902+-3.3878        ? might be 1.0297x slower
   destructuring-parameters-overridden-by-function   
                                                      0.4166+-0.0346            0.4023+-0.0131          might be 1.0356x faster
   destructuring-swap                                 4.4747+-0.1735            4.4496+-0.1212        
   direct-arguments-getbyval                          1.0979+-0.0769            1.0839+-0.0377          might be 1.0130x faster
   div-boolean-double                                 4.9726+-0.0276     ?      4.9812+-0.0438        ?
   div-boolean                                        7.6039+-0.0222     !      7.7957+-0.1528        ! definitely 1.0252x slower
   double-get-by-val-out-of-bounds                    4.0640+-0.1432            4.0357+-0.1371        
   double-pollution-getbyval                          8.2465+-0.1908            8.0736+-0.0266          might be 1.0214x faster
   double-pollution-putbyoffset                       3.7277+-0.0815            3.6359+-0.1074          might be 1.0252x faster
   double-real-use                                   25.1392+-1.7264     ?     25.1742+-1.0249        ?
   double-to-int32-typed-array-no-inline              2.0137+-0.1775            1.9539+-0.0752          might be 1.0307x faster
   double-to-int32-typed-array                        1.6498+-0.0332     ^      1.5824+-0.0256        ^ definitely 1.0426x faster
   double-to-uint32-typed-array-no-inline             2.0000+-0.1697            1.9797+-0.0372          might be 1.0103x faster
   double-to-uint32-typed-array                       1.6745+-0.0380     ?      1.7005+-0.0382        ? might be 1.0155x slower
   elidable-new-object-dag                           31.9268+-0.5333           31.5892+-0.2469          might be 1.0107x faster
   elidable-new-object-roflcopter                    31.3093+-0.6566           30.8390+-0.2394          might be 1.0153x faster
   elidable-new-object-then-call                     28.9281+-0.5756     ?     29.6153+-0.3816        ? might be 1.0238x slower
   elidable-new-object-tree                          33.6996+-0.2418     ?     33.9793+-0.8549        ?
   empty-string-plus-int                              4.5655+-0.1366            4.4914+-0.0714          might be 1.0165x faster
   emscripten-cube2hash                              25.0409+-0.7810           24.7831+-0.7891          might be 1.0104x faster
   exit-length-on-plain-object                       11.3990+-0.1520           11.3844+-0.2831        
   external-arguments-getbyval                        1.0668+-0.0312     ?      1.1283+-0.1025        ? might be 1.0576x slower
   external-arguments-putbyval                        1.9745+-0.0862     ?      2.1868+-0.1627        ? might be 1.1075x slower
   fixed-typed-array-storage-var-index                1.1467+-0.1973     ?      1.1487+-0.1034        ?
   fixed-typed-array-storage                          0.8233+-0.0881            0.7751+-0.0192          might be 1.0621x faster
   Float32Array-matrix-mult                           3.6772+-0.1447     ?      3.7769+-0.2437        ? might be 1.0271x slower
   Float32Array-to-Float64Array-set                  43.9920+-0.2286           43.6979+-0.1440        
   Float64Array-alloc-long-lived                     51.6110+-0.4885     ?     51.6566+-0.4290        ?
   Float64Array-to-Int16Array-set                    52.3091+-1.2126     ?     53.7286+-2.0918        ? might be 1.0271x slower
   fold-double-to-int                                19.2090+-0.5553     ?     20.6349+-0.9481        ? might be 1.0742x slower
   fold-get-by-id-to-multi-get-by-offset-rare-int   
                                                      9.6695+-0.2880            9.5391+-0.2414          might be 1.0137x faster
   fold-get-by-id-to-multi-get-by-offset              7.7612+-0.1588            7.7569+-0.2031        
   fold-multi-get-by-offset-to-get-by-offset   
                                                      7.7154+-0.3914            7.5130+-0.6271          might be 1.0269x faster
   fold-multi-get-by-offset-to-poly-get-by-offset   
                                                      7.4245+-1.3568            7.1127+-0.2703          might be 1.0438x faster
   fold-multi-put-by-offset-to-poly-put-by-offset   
                                                      6.8127+-0.2884     ?      7.1256+-0.2024        ? might be 1.0459x slower
   fold-multi-put-by-offset-to-put-by-offset   
                                                      4.7530+-0.5890            4.5177+-0.4992          might be 1.0521x faster
   fold-multi-put-by-offset-to-replace-or-transition-put-by-offset   
                                                      7.8578+-0.2644            7.7592+-0.2125          might be 1.0127x faster
   fold-put-by-id-to-multi-put-by-offset              8.2598+-0.3062            8.0034+-0.3075          might be 1.0320x faster
   fold-put-structure                                 4.8517+-0.2778            4.8491+-0.3488        
   for-of-iterate-array-entries                      10.8067+-0.1348           10.5767+-0.1434          might be 1.0217x faster
   for-of-iterate-array-keys                          3.2344+-0.0887     ?      3.2928+-0.1599        ? might be 1.0181x slower
   for-of-iterate-array-values                        3.1790+-0.1198     ?      3.2430+-0.1006        ? might be 1.0201x slower
   fround                                            17.4968+-0.5395     ?     17.5215+-0.7143        ?
   ftl-library-inlining-dataview                     52.4436+-0.0849     ?     53.4190+-1.0113        ? might be 1.0186x slower
   ftl-library-inlining                             101.9279+-0.1381          101.8230+-0.1294        
   function-dot-apply                                 1.8115+-0.0256     ?      1.8322+-0.0338        ? might be 1.0114x slower
   function-test                                      2.6131+-0.1576            2.5140+-0.0366          might be 1.0394x faster
   function-with-eval                                84.8065+-1.2265     ?     85.5884+-1.3837        ?
   gcse-poly-get-less-obvious                        13.5126+-0.2982     ?     13.9456+-0.8226        ? might be 1.0320x slower
   gcse-poly-get                                     13.2464+-0.1238     ?     13.3517+-0.1815        ?
   gcse                                               3.6292+-0.1626     ?      3.7944+-0.4241        ? might be 1.0455x slower
   get-by-id-bimorphic-check-structure-elimination-simple   
                                                      2.4732+-0.1465            2.4297+-0.0433          might be 1.0179x faster
   get-by-id-bimorphic-check-structure-elimination   
                                                      5.3035+-0.0696     ?      5.4663+-0.1822        ? might be 1.0307x slower
   get-by-id-chain-from-try-block                     6.1245+-0.2304            6.0893+-0.2689        
   get-by-id-check-structure-elimination              4.1070+-0.0877     ?      4.4592+-0.8889        ? might be 1.0858x slower
   get-by-id-proto-or-self                           13.3574+-0.3570     ?     13.7417+-0.6373        ? might be 1.0288x slower
   get-by-id-quadmorphic-check-structure-elimination-simple   
                                                      2.6694+-0.0341     ?      2.7422+-0.1190        ? might be 1.0273x slower
   get-by-id-self-or-proto                           13.7190+-0.5600     ?     14.2226+-0.7426        ? might be 1.0367x slower
   get-by-val-out-of-bounds                           3.9059+-0.1713            3.8364+-0.0983          might be 1.0181x faster
   get_callee_monomorphic                             2.2092+-0.1094            2.1218+-0.0809          might be 1.0412x faster
   get_callee_polymorphic                             3.8713+-0.2007            3.8704+-0.0595        
   getter-no-activation                               4.5368+-0.0938     ?      4.5385+-0.0713        ?
   getter-prototype                                   9.0212+-0.0559     ?      9.3590+-0.5628        ? might be 1.0374x slower
   getter-richards                                  104.1934+-2.4926          101.9770+-4.8311          might be 1.0217x faster
   getter                                             5.1221+-0.3074            4.9872+-0.2655          might be 1.0271x faster
   global-var-const-infer-fire-from-opt               0.8218+-0.0520     ?      0.8481+-0.0490        ? might be 1.0320x slower
   global-var-const-infer                             0.6606+-0.0314            0.6579+-0.0168        
   HashMap-put-get-iterate-keys                      24.7713+-0.9060           24.7565+-1.0024        
   HashMap-put-get-iterate                           24.5078+-0.6161     ?     25.1520+-1.0318        ? might be 1.0263x slower
   HashMap-string-put-get-iterate                    22.9903+-0.3983           22.4579+-0.4710          might be 1.0237x faster
   hoist-make-rope                                    7.8741+-0.5129            7.3785+-0.2748          might be 1.0672x faster
   hoist-poly-check-structure-effectful-loop   
                                                      3.9684+-0.1127            3.9636+-0.1402        
   hoist-poly-check-structure                         3.0463+-0.0364     ?      3.1791+-0.2239        ? might be 1.0436x slower
   imul-double-only                                   6.9848+-1.3416            6.5314+-0.1906          might be 1.0694x faster
   imul-int-only                                      7.6119+-0.5439     ?      8.0976+-0.9350        ? might be 1.0638x slower
   imul-mixed                                         6.2379+-0.3432            6.0171+-0.2686          might be 1.0367x faster
   in-four-cases                                     16.0776+-0.2378     ?     16.2493+-0.4314        ? might be 1.0107x slower
   in-one-case-false                                  8.8411+-0.1894     ?      8.9008+-0.2487        ?
   in-one-case-true                                   8.9097+-0.2563            8.8397+-0.1780        
   in-two-cases                                       9.1547+-0.1615     ?      9.1771+-0.1813        ?
   indexed-properties-in-objects                      2.7377+-0.1017     ?      2.7678+-0.1099        ? might be 1.0110x slower
   infer-closure-const-then-mov-no-inline             2.9617+-0.0332            2.8899+-0.0561          might be 1.0249x faster
   infer-closure-const-then-mov                      15.7995+-0.2882     ?     16.6623+-0.6481        ? might be 1.0546x slower
   infer-closure-const-then-put-to-scope-no-inline   
                                                     10.7274+-0.0993     ?     10.8715+-0.2385        ? might be 1.0134x slower
   infer-closure-const-then-put-to-scope             20.5397+-0.7053     ?     21.2623+-0.9339        ? might be 1.0352x slower
   infer-closure-const-then-reenter-no-inline   
                                                     44.8694+-1.5512           44.2460+-0.1574          might be 1.0141x faster
   infer-closure-const-then-reenter                  20.7467+-0.5922           20.7435+-0.4790        
   infer-constant-global-property                     3.1518+-0.0609            3.1477+-0.0516        
   infer-constant-property                            2.4378+-0.0530     ?      2.4887+-0.1105        ? might be 1.0209x slower
   infer-one-time-closure-ten-vars                    7.8804+-0.2160     ?      8.1454+-0.3753        ? might be 1.0336x slower
   infer-one-time-closure-two-vars                    7.5693+-0.3275            7.4586+-0.1153          might be 1.0148x faster
   infer-one-time-closure                             7.2594+-0.0868     ?      7.2839+-0.0876        ?
   infer-one-time-deep-closure                       12.1854+-0.4718           12.1799+-0.3049        
   inline-arguments-access                            3.5047+-0.1497            3.4413+-0.1222          might be 1.0184x faster
   inline-arguments-aliased-access                    3.5512+-0.1522     ?      3.5996+-0.1972        ? might be 1.0136x slower
   inline-arguments-local-escape                      3.7052+-0.3125            3.5268+-0.1591          might be 1.0506x faster
   inline-get-scoped-var                              4.2989+-0.0604            4.2895+-0.0663        
   inlined-put-by-id-transition                       9.3763+-0.3978            9.3362+-0.4241        
   int-or-other-abs-then-get-by-val                   4.4881+-0.1039            4.4700+-0.0666        
   int-or-other-abs-zero-then-get-by-val             15.2324+-0.4987     ?     15.4580+-0.8923        ? might be 1.0148x slower
   int-or-other-add-then-get-by-val                   3.8216+-0.1271            3.7758+-0.0517          might be 1.0121x faster
   int-or-other-add                                   4.6131+-0.1208            4.6053+-0.0607        
   int-or-other-div-then-get-by-val                   3.4965+-0.1005            3.4424+-0.0586          might be 1.0157x faster
   int-or-other-max-then-get-by-val                   3.6572+-0.0568     ?      3.7002+-0.1121        ? might be 1.0118x slower
   int-or-other-min-then-get-by-val                   3.6396+-0.0455     ?      3.8209+-0.2032        ? might be 1.0498x slower
   int-or-other-mod-then-get-by-val                   3.2513+-0.0552     ?      3.3400+-0.2425        ? might be 1.0273x slower
   int-or-other-mul-then-get-by-val                   3.4610+-0.1585            3.3894+-0.0728          might be 1.0211x faster
   int-or-other-neg-then-get-by-val                   4.1865+-0.0601     ?      4.2265+-0.0526        ?
   int-or-other-neg-zero-then-get-by-val             15.1306+-0.1894     ?     15.5741+-0.8991        ? might be 1.0293x slower
   int-or-other-sub-then-get-by-val                   3.8086+-0.0766     ?      3.8113+-0.0685        ?
   int-or-other-sub                                   3.2255+-0.1396            3.1701+-0.0426          might be 1.0175x faster
   int-overflow-local                                 4.0112+-0.0179     ?      4.0976+-0.1649        ? might be 1.0215x slower
   Int16Array-alloc-long-lived                       38.6375+-0.4509     ?     38.7746+-0.4770        ?
   Int16Array-bubble-sort-with-byteLength            12.6104+-0.4623           12.3749+-0.1511          might be 1.0190x faster
   Int16Array-bubble-sort                            13.1835+-0.4946     ?     13.2486+-0.1704        ?
   Int16Array-load-int-mul                            1.2896+-0.0355     ?      1.2975+-0.0096        ?
   Int16Array-to-Int32Array-set                      42.9758+-1.0294           42.5314+-0.1267          might be 1.0104x faster
   Int32Array-alloc-large                            11.5908+-0.3674           11.3182+-0.2022          might be 1.0241x faster
   Int32Array-alloc-long-lived                       42.9333+-0.5323     ?     43.1819+-0.6809        ?
   Int32Array-alloc                                   2.7630+-0.1469     ?      2.7812+-0.2129        ?
   Int32Array-Int8Array-view-alloc                    5.6915+-0.2977     ?      5.8240+-0.1787        ? might be 1.0233x slower
   int52-spill                                        4.3835+-0.3787            4.2027+-0.0420          might be 1.0430x faster
   Int8Array-alloc-long-lived                        34.2881+-0.3877           34.2436+-0.3732        
   Int8Array-load-with-byteLength                     3.1529+-0.1344     ?      3.1718+-0.1189        ?
   Int8Array-load                                     3.1830+-0.1408     ?      3.2201+-0.1417        ? might be 1.0116x slower
   integer-divide                                     9.5198+-0.1352     ?      9.5471+-0.1184        ?
   integer-modulo                                     1.5128+-0.0423     ?      1.5408+-0.0287        ? might be 1.0185x slower
   is-boolean-fold-tricky                             3.5089+-0.0449     ?      3.7020+-0.2943        ? might be 1.0550x slower
   is-boolean-fold                                    2.4335+-0.0210     ?      2.4464+-0.0328        ?
   is-function-fold-tricky-internal-function   
                                                      9.5350+-0.2043            9.3833+-0.1547          might be 1.0162x faster
   is-function-fold-tricky                            3.7746+-0.0276     ?      3.8557+-0.1112        ? might be 1.0215x slower
   is-function-fold                                   2.5413+-0.1629            2.4573+-0.0390          might be 1.0342x faster
   is-number-fold-tricky                              3.7715+-0.0492     ?      3.7785+-0.1258        ?
   is-number-fold                                     2.4906+-0.1352            2.4489+-0.0352          might be 1.0170x faster
   is-object-or-null-fold-functions                   2.5126+-0.1154            2.5094+-0.0746        
   is-object-or-null-fold-less-tricky                 3.8714+-0.1457            3.8410+-0.1373        
   is-object-or-null-fold-tricky                      4.8780+-0.0634     ?      4.8892+-0.0547        ?
   is-object-or-null-fold                             2.4507+-0.0550            2.4457+-0.0356        
   is-object-or-null-trickier-function                3.9225+-0.1682            3.7899+-0.0411          might be 1.0350x faster
   is-object-or-null-trickier-internal-function   
                                                      9.6929+-0.1514     ?     10.0349+-0.2370        ? might be 1.0353x slower
   is-object-or-null-tricky-function                  4.1281+-0.6599            3.8907+-0.1062          might be 1.0610x faster
   is-object-or-null-tricky-internal-function   
                                                      7.1997+-0.1418     ?      7.2688+-0.2463        ?
   is-string-fold-tricky                              3.9238+-0.1959            3.8171+-0.1713          might be 1.0279x faster
   is-string-fold                                     2.4469+-0.0417            2.4169+-0.0207          might be 1.0124x faster
   is-undefined-fold-tricky                           3.1391+-0.1135            3.0469+-0.0231          might be 1.0303x faster
   is-undefined-fold                                  2.4232+-0.0382     ?      2.4327+-0.0358        ?
   large-int-captured                                 3.8020+-0.2188            3.7343+-0.1516          might be 1.0181x faster
   large-int-neg                                     13.4199+-0.4516     ?     13.5736+-0.7687        ? might be 1.0115x slower
   large-int                                         13.0223+-0.9567           12.9525+-0.4737        
   load-varargs-elimination                          20.9050+-0.8370     ^     19.5354+-0.3650        ^ definitely 1.0701x faster
   logical-not-weird-types                            2.7437+-0.1312            2.6446+-0.0570          might be 1.0375x faster
   logical-not                                        3.9924+-0.0703            3.9890+-0.0615        
   lots-of-fields                                     8.9755+-0.1484     ?      9.0172+-0.2138        ?
   make-indexed-storage                               2.7729+-0.2101     ?      2.7783+-0.1532        ?
   make-rope-cse                                      3.5535+-0.2019            3.4209+-0.0452          might be 1.0388x faster
   marsaglia-larger-ints                             30.2034+-0.3104     ?     30.7635+-0.5266        ? might be 1.0185x slower
   marsaglia-osr-entry                               19.7925+-0.5044     ?     20.2801+-0.6388        ? might be 1.0246x slower
   math-with-out-of-bounds-array-values              21.1441+-0.9695     ?     21.1688+-1.0850        ?
   max-boolean                                        2.4989+-0.0701     ?      2.5308+-0.1060        ? might be 1.0128x slower
   method-on-number                                  15.1748+-0.3305           14.9151+-0.2431          might be 1.0174x faster
   min-boolean                                        2.4289+-0.0681     ?      2.4440+-0.0418        ?
   minus-boolean-double                               2.8325+-0.0365     ?      2.8573+-0.0387        ?
   minus-boolean                                      2.1495+-0.0325     ?      2.2842+-0.1906        ? might be 1.0626x slower
   misc-strict-eq                                    29.6075+-1.3612           29.0054+-0.6566          might be 1.0208x faster
   mod-boolean-double                                10.4131+-0.1404     ?     10.4819+-0.1813        ?
   mod-boolean                                        7.7367+-0.0223     ?      7.8029+-0.1201        ?
   mul-boolean-double                                 3.4039+-0.1289     ?      3.4644+-0.1622        ? might be 1.0178x slower
   mul-boolean                                        2.5641+-0.0270     ?      2.6689+-0.1087        ? might be 1.0409x slower
   neg-boolean                                        2.8189+-0.0281     ?      2.8886+-0.0430        ? might be 1.0247x slower
   negative-zero-divide                               0.2896+-0.0087     ?      0.2984+-0.0080        ? might be 1.0304x slower
   negative-zero-modulo                               0.2839+-0.0101     ?      0.2972+-0.0087        ? might be 1.0470x slower
   negative-zero-negate                               0.2705+-0.0087     ?      0.2819+-0.0105        ? might be 1.0421x slower
   nested-function-parsing                           31.3804+-0.3602     !     42.9210+-5.4653        ! definitely 1.3678x slower
   new-array-buffer-dead                             81.5850+-0.7304     ?     81.6318+-0.4643        ?
   new-array-buffer-push                              5.4375+-0.1523     ?      5.6137+-0.2480        ? might be 1.0324x slower
   new-array-dead                                    13.4624+-0.5506     ?     13.5344+-0.6621        ?
   new-array-push                                     3.3297+-0.2115            3.1814+-0.0431          might be 1.0466x faster
   no-inline-constructor                             29.1634+-0.1208     ?     29.6429+-0.7908        ? might be 1.0164x slower
   number-test                                        2.6365+-0.0466     ?      2.7534+-0.1646        ? might be 1.0443x slower
   object-closure-call                                4.6523+-0.1949            4.5139+-0.0547          might be 1.0307x faster
   object-test                                        2.4474+-0.0425     ?      2.4577+-0.0746        ?
   obvious-sink-pathology-taken                      94.8104+-0.8258           94.7313+-0.6156        
   obvious-sink-pathology                            90.5305+-0.4601     ?     90.9384+-0.9688        ?
   obviously-elidable-new-object                     27.0934+-0.6021           25.8716+-0.8949          might be 1.0472x faster
   plus-boolean-arith                                 2.2054+-0.0513            2.2029+-0.0402        
   plus-boolean-double                                2.8798+-0.1047     ?      3.0254+-0.2171        ? might be 1.0506x slower
   plus-boolean                                       2.3616+-0.0322     ?      2.3737+-0.0403        ?
   poly-chain-access-different-prototypes-simple   
                                                      2.5150+-0.0207     ?      2.5462+-0.0596        ? might be 1.0124x slower
   poly-chain-access-different-prototypes             2.3939+-0.0884            2.3509+-0.0497          might be 1.0183x faster
   poly-chain-access-simpler                          2.5244+-0.0521            2.5114+-0.0345        
   poly-chain-access                                  2.3206+-0.0298     ?      2.3461+-0.0646        ? might be 1.0110x slower
   poly-stricteq                                     47.3038+-0.1588     ^     46.4318+-0.0959        ^ definitely 1.0188x faster
   polymorphic-array-call                             1.1876+-0.0595            1.1594+-0.0377          might be 1.0243x faster
   polymorphic-get-by-id                              2.5652+-0.0263     ?      2.6072+-0.0450        ? might be 1.0164x slower
   polymorphic-put-by-id                             25.0882+-1.2696           23.5852+-0.5526          might be 1.0637x faster
   polymorphic-structure                             12.3984+-0.1931           12.3661+-0.1931        
   polyvariant-monomorphic-get-by-id                  6.2292+-0.2810            6.2058+-0.4175        
   proto-getter-access                                7.6247+-0.2222            7.5997+-0.1996        
   put-by-id-replace-and-transition                   7.1574+-0.2513     ?      7.2650+-0.3394        ? might be 1.0150x slower
   put-by-id-slightly-polymorphic                     2.4568+-0.0280     ?      2.6291+-0.2725        ? might be 1.0701x slower
   put-by-id                                          9.2477+-0.4114            9.2245+-0.2392        
   put-by-val-direct                                  0.2855+-0.0100     ?      0.3182+-0.0652        ? might be 1.1143x slower
   put-by-val-large-index-blank-indexing-type   
                                                      4.8449+-0.1167            4.7809+-0.1608          might be 1.0134x faster
   put-by-val-machine-int                             2.3091+-0.0401     ?      2.3103+-0.0754        ?
   rare-osr-exit-on-local                            13.7144+-0.1481     ?     13.7670+-0.1291        ?
   register-pressure-from-osr                        15.3552+-0.2005           15.2557+-0.2143        
   repeat-multi-get-by-offset                        20.2082+-0.5380     ?     21.3747+-1.0337        ? might be 1.0577x slower
   setter-prototype                                   6.8568+-0.1638     ?      6.9148+-0.1717        ?
   setter                                             5.1500+-0.2727     ?      5.1569+-0.3469        ?
   simple-activation-demo                            23.0027+-0.5058     ?     23.4347+-1.0047        ? might be 1.0188x slower
   simple-getter-access                               9.8567+-0.2446            9.7732+-0.1124        
   simple-poly-call-nested                            7.9158+-0.4196            7.7312+-0.3420          might be 1.0239x faster
   simple-poly-call                                   1.1693+-0.0535            1.1277+-0.0342          might be 1.0369x faster
   sin-boolean                                       17.9292+-1.5351           16.9934+-0.9044          might be 1.0551x faster
   singleton-scope                                   51.4835+-0.1117     !     56.3212+-0.0958        ! definitely 1.0940x slower
   sink-function                                      8.9132+-0.4590            8.7557+-0.1783          might be 1.0180x faster
   sink-huge-activation                              15.4365+-0.5881           15.3177+-0.5743        
   sinkable-new-object-dag                           52.4115+-0.5978     ?     54.9851+-6.5508        ? might be 1.0491x slower
   sinkable-new-object-taken                         39.5195+-0.4385           39.2385+-0.3837        
   sinkable-new-object                               27.7583+-0.3024           27.6940+-0.4109        
   slow-array-profile-convergence                     2.3041+-0.1285     ?      2.4146+-0.2124        ? might be 1.0480x slower
   slow-convergence                                   2.2738+-0.1457     ?      2.2879+-0.1220        ?
   slow-ternaries                                    16.4343+-0.3143     ?     16.5933+-0.4643        ?
   sorting-benchmark                                 16.2930+-0.4218           16.2368+-0.3852        
   sparse-conditional                                 1.0166+-0.0211            1.0161+-0.0081        
   splice-to-remove                                  11.7135+-0.6334     ?     11.7355+-0.1543        ?
   string-char-code-at                               13.5527+-0.2526     ?     13.6118+-0.4492        ?
   string-concat-object                               2.1017+-0.1711     ?      2.1023+-0.1141        ?
   string-concat-pair-object                          2.0176+-0.0891     ?      2.0779+-0.1615        ? might be 1.0299x slower
   string-concat-pair-simple                          8.8998+-0.3767            8.5811+-0.1917          might be 1.0371x faster
   string-concat-simple                               8.5866+-0.1240     ?      8.8769+-0.3122        ? might be 1.0338x slower
   string-cons-repeat                                 6.5421+-0.6480            6.2478+-0.3565          might be 1.0471x faster
   string-cons-tower                                  6.6026+-0.5832            6.2934+-0.1708          might be 1.0491x faster
   string-equality                                   14.4744+-0.5487           14.3960+-0.6464        
   string-get-by-val-big-char                         6.2472+-0.0841     ^      6.1074+-0.0470        ^ definitely 1.0229x faster
   string-get-by-val-out-of-bounds-insane             3.0917+-0.0979            2.9832+-0.0789          might be 1.0364x faster
   string-get-by-val-out-of-bounds                    3.8064+-0.0545     ?      3.9104+-0.1768        ? might be 1.0273x slower
   string-get-by-val                                  2.5949+-0.0339     ?      2.6095+-0.0401        ?
   string-hash                                        1.7914+-0.0224     ?      1.8476+-0.1397        ? might be 1.0314x slower
   string-long-ident-equality                        11.8462+-0.1330           11.6518+-0.0731          might be 1.0167x faster
   string-out-of-bounds                               9.8500+-0.2339            9.8186+-0.2830        
   string-repeat-arith                               26.1596+-0.3878           25.5968+-0.4253          might be 1.0220x faster
   string-sub                                        51.0924+-0.5146     ^     49.5492+-0.2559        ^ definitely 1.0311x faster
   string-test                                        2.6573+-0.1616            2.6379+-0.0936        
   string-var-equality                               23.9200+-0.9319     ?     24.0679+-0.9793        ?
   structure-hoist-over-transitions                   2.3069+-0.0433     ?      2.4072+-0.1381        ? might be 1.0434x slower
   substring-concat-weird                            34.1333+-0.4962           33.6913+-0.5623          might be 1.0131x faster
   substring-concat                                  37.0259+-0.5646     ?     38.4126+-3.3106        ? might be 1.0375x slower
   substring                                         42.4044+-0.2980           42.3713+-0.2903        
   switch-char-constant                               2.5397+-0.0634     ?      2.6037+-0.1547        ? might be 1.0252x slower
   switch-char                                        5.0670+-0.2154     ?      5.2149+-0.3893        ? might be 1.0292x slower
   switch-constant                                    7.5514+-0.5173     ?      7.7464+-0.5688        ? might be 1.0258x slower
   switch-string-basic-big-var                       14.7285+-0.2386     ?     14.9864+-0.5707        ? might be 1.0175x slower
   switch-string-basic-big                           14.0131+-0.1817     ?     14.5388+-1.0274        ? might be 1.0375x slower
   switch-string-basic-var                           12.4377+-0.2870     ?     12.6099+-0.5863        ? might be 1.0138x slower
   switch-string-basic                               12.1934+-0.3312     ?     12.3273+-0.2598        ? might be 1.0110x slower
   switch-string-big-length-tower-var                16.7375+-0.4138     ?     17.0145+-0.5221        ? might be 1.0165x slower
   switch-string-length-tower-var                    12.3886+-0.3512     ?     12.4980+-0.5224        ?
   switch-string-length-tower                        11.0312+-0.1443     ?     11.2332+-0.3467        ? might be 1.0183x slower
   switch-string-short                               10.9579+-0.1405     ?     11.2660+-0.2880        ? might be 1.0281x slower
   switch                                            11.5705+-0.5873     ?     12.4178+-1.3499        ? might be 1.0732x slower
   tear-off-arguments-simple                          3.1084+-0.3316            2.9188+-0.0980          might be 1.0650x faster
   tear-off-arguments                                 3.9411+-0.1184            3.8403+-0.1140          might be 1.0263x faster
   temporal-structure                                11.4678+-0.3862     ?     11.5089+-0.4085        ?
   to-int32-boolean                                  12.3720+-0.2023     ?     12.6457+-0.4743        ? might be 1.0221x slower
   try-catch-get-by-val-cloned-arguments             12.1970+-0.3871     ?     12.4668+-0.5785        ? might be 1.0221x slower
   try-catch-get-by-val-direct-arguments              5.4793+-0.2761            5.4459+-0.1468        
   try-catch-get-by-val-scoped-arguments              6.2072+-0.1091     ?      6.3717+-0.2217        ? might be 1.0265x slower
   typed-array-get-set-by-val-profiling              25.8771+-0.4595     ?     25.8860+-0.3395        ?
   undefined-property-access                        203.1529+-2.4931          201.9467+-1.0111        
   undefined-test                                     2.6594+-0.0612     ?      2.7923+-0.1997        ? might be 1.0500x slower
   unprofiled-licm                                   13.5990+-0.8721           13.0716+-0.1564          might be 1.0403x faster
   varargs-call                                      12.6834+-0.2512           12.5922+-0.1344        
   varargs-construct-inline                          21.1715+-0.7266           20.8039+-0.1363          might be 1.0177x faster
   varargs-construct                                 18.8069+-0.2651     ?     19.3352+-0.3020        ? might be 1.0281x slower
   varargs-inline                                     7.7959+-0.1306            7.7160+-0.0421          might be 1.0103x faster
   varargs-strict-mode                                8.6720+-0.2676            8.5572+-0.1944          might be 1.0134x faster
   varargs                                            8.4223+-0.0668     ?      9.3224+-1.7417        ? might be 1.1069x slower
   weird-inlining-const-prop                          2.1225+-0.0850            2.0609+-0.0791          might be 1.0299x faster

   <geometric>                                        7.2502+-0.0139     ?      7.2762+-0.0123        ? might be 1.0036x slower

                                                         original                 letScoping                                    
CompressionBench:
   huffman                                           50.8516+-2.0289     ?     50.8771+-1.0365        ?
   arithmetic-simple                                248.7730+-1.2212     ?    251.2240+-3.0965        ?
   arithmetic-precise                               228.5495+-1.3075          228.0620+-0.9488        
   arithmetic-complex-precise                       230.4627+-1.2529     ?    231.0217+-4.6117        ?
   arithmetic-precise-order-0                       255.1887+-1.7319     ?    255.8374+-1.3974        ?
   arithmetic-precise-order-1                       285.9208+-7.5789          279.7760+-0.6574          might be 1.0220x faster
   arithmetic-precise-order-2                       326.7656+-2.8365          326.0814+-2.1519        
   arithmetic-simple-order-1                        300.9293+-1.5797          300.0423+-1.4807        
   arithmetic-simple-order-2                        354.0885+-3.6161     ?    354.5633+-3.2335        ?
   lz-string                                        299.8130+-3.3722     ?    304.5122+-8.9896        ? might be 1.0157x slower

   <geometric>                                      234.6706+-1.5896     ?    234.7577+-1.1152        ? might be 1.0004x slower

                                                         original                 letScoping                                    
Geomean of preferred means:
   <scaled-result>                                   33.3781+-0.1317     ?     33.5102+-0.0948        ? might be 1.0040x slower

[Saam Barati]

(In reply to comment #129)
> Comment on attachment 256658 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256658&action=review
> 
> > Source/JavaScriptCore/bytecode/EvalCodeCache.h:79
> > +            // If eval() is called and it has access to a lexical scope, we can't soundly cache it.
> 
> Why?
For this situation:
function foo() {
    let x = 20;
    eval("x;");
    if (b) {
        let x = 30;
        if (b) {
            let y = 40;
            eval("x;")
        }
    }
}

We can't reuse resolution depth when linking get_from_scope in evals.

> 
> > Source/JavaScriptCore/dfg/DFGOperations.cpp:765
> > -JSCell* JIT_OPERATION operationCreateActivationDirect(ExecState* exec, Structure* structure, JSScope* scope, SymbolTable* table)
> > +JSCell* JIT_OPERATION operationPushLexicalEnvironmentDirect(ExecState* exec, Structure* structure, JSScope* scope, SymbolTable* table, EncodedJSValue initialValueEncoded)
> 
> Seems like this should be called CreateActivation not PushLexicalEnvironment.
Will revert.

> 
> > Source/JavaScriptCore/jit/JITOperations.cpp:1446
> > +EncodedJSValue JIT_OPERATION operationPushLexicalEnvironment(ExecState* exec, Instruction* bytecodePC)
> 
> This also seems like it's creating, not pushing.
Will revert.

> 
> > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1294
> > +LLINT_SLOW_PATH_DECL(slow_path_create_lexical_environment)
> 
> You could make this a CommonSlowPath and have the JIT call it, which would
> obviate the need for a Create operation in JITOperations.
Okay I'll do this.

[Saam Barati]

Created attachment 256860 [details]
patch for landing

[WebKit Commit Bot]

Attachment 256860 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/ParserTokens.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:525:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1323:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2483:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2946:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/parser/Keywords.table:22:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 6 in 90 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/186860

[Yusuke Suzuki]

(In reply to comment #135)
> landed in:
> http://trac.webkit.org/changeset/186860

Congrats!!