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.
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]();
}
```
(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.
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();
}
})();
```
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; }
}
```
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.
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?
(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
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.
(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.
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.
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.
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
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.
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.
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.
(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.
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.
(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.
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.
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.
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.
(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"
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.
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).
(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.
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 :(.
(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.
> > > 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.
(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?
> > 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.
> 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.
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.
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.
(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.
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.
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.
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.
(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.
(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.
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.
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
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
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.
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
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
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.
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.
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.
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.
(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.
> 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.
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.
(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.
(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.
(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.
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.
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.
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
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
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.
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
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
> 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.)
> > 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.
> > (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.
> 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).
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.
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"); }
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.
(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.
(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.
(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.
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.
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.
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
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
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.
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.
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.
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.
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.
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.
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.
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.
(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.
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.
2015-04-01 15:08 PDT, Saam Barati
2015-04-01 21:44 PDT, Saam Barati
2015-04-05 14:51 PDT, Saam Barati
2015-04-06 15:01 PDT, Saam Barati
2015-04-10 14:00 PDT, Saam Barati
2015-04-13 17:48 PDT, Saam Barati
2015-04-14 10:52 PDT, Saam Barati
2015-04-14 10:53 PDT, Saam Barati
2015-04-17 21:24 PDT, Saam Barati
2015-04-19 15:06 PDT, Saam Barati
2015-04-19 15:56 PDT, Build Bot
2015-04-19 16:32 PDT, Saam Barati
2015-04-22 22:47 PDT, Saam Barati
2015-04-25 16:27 PDT, Saam Barati
2015-04-28 18:56 PDT, Saam Barati
2015-04-30 11:37 PDT, Saam Barati
2015-05-13 15:23 PDT, Saam Barati
2015-05-13 16:03 PDT, Saam Barati
2015-05-14 02:25 PDT, Saam Barati
2015-05-27 10:56 PDT, Saam Barati
2015-06-24 18:45 PDT, Saam Barati
2015-06-25 19:42 PDT, Saam Barati
2015-06-26 01:54 PDT, Saam Barati
2015-06-29 18:35 PDT, Saam Barati
2015-06-29 19:32 PDT, Build Bot
2015-06-29 19:55 PDT, Build Bot
2015-06-30 10:41 PDT, Saam Barati
2015-06-30 11:33 PDT, Build Bot
2015-06-30 11:39 PDT, Build Bot
2015-06-30 16:28 PDT, Saam Barati
2015-07-02 14:00 PDT, Saam Barati
2015-07-02 14:18 PDT, Saam Barati
2015-07-06 11:41 PDT, Saam Barati
2015-07-06 12:45 PDT, Build Bot
2015-07-06 12:47 PDT, Build Bot
2015-07-06 16:49 PDT, Saam Barati
2015-07-06 17:54 PDT, Build Bot
2015-07-06 17:57 PDT, Build Bot
2015-07-08 17:18 PDT, Saam Barati
2015-07-08 17:25 PDT, Saam Barati
2015-07-08 18:45 PDT, Build Bot
2015-07-08 19:04 PDT, Build Bot
2015-07-09 00:06 PDT, Saam Barati
2015-07-09 08:21 PDT, Saam Barati
2015-07-11 11:03 PDT, Saam Barati
2015-07-15 13:56 PDT, Saam Barati