The spec: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-let-and-const-declarations
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 249961 [details] WIP defined new op code.
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.
Created attachment 250684 [details] WIP working in loops.
Created attachment 250714 [details] WIP Rebased yesterday.
Created attachment 250715 [details] perf tests
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
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.
Created attachment 251130 [details] WIP exception handling done.
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.
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
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
Created attachment 251136 [details] WIP remove print statements from previous patch
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.
Created attachment 251653 [details] WIP
Created attachment 251913 [details] rebased patch
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.
Created attachment 252074 [details] patch pretty close to being done. Just waiting on blocking patches.
(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.
Created attachment 253066 [details] patch
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"
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
Created attachment 253070 [details] patch
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.
Oh man, that's a juicy patch. Looking now.
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).
Created attachment 253115 [details] patch Add TDZ check for destructuring assignment expressions: ``` [x] = [10]; let x; ```
(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 Looks like this needs rebasing.
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.
*** Bug 141885 has been marked as a duplicate of this bug. ***
(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.
Created attachment 255536 [details] patch
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.
Created attachment 255622 [details] perf results Looks neutral, as expected.
(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.
Created attachment 255804 [details] patch
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.
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
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
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
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
Created attachment 255830 [details] patch Lets see if EWS likes this patch better.
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.
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
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
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
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
Created attachment 255871 [details] patch
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.
Created attachment 256033 [details] patch Added a Parser environment data structure to represent a parse/bytecode compile time variable environment
*** Bug 144978 has been marked as a duplicate of this bug. ***
> ERROR: Source/JavaScriptCore/ChangeLog:152: Line contains tab character. > [whitespace/tab] [5] Please fix.
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.
Created attachment 256036 [details] patch fixed needed style issues.
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.
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.
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
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.
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
Created attachment 256257 [details] patch
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.
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.
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
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.
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
Looks like this still needs some bug fixing.
> 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.
Also: All uses of isVariableObject() are wrong in the world of let, and you should fix them.
(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.
Created attachment 256433 [details] patch
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.
Created attachment 256438 [details] patch same patch as before but with a comment to clarify one bit of code.
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.
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.
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
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.
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
Created attachment 256465 [details] patch
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.
Created attachment 256481 [details] patch fix armv7 build.
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.
*** Bug 144979 has been marked as a duplicate of this bug. ***
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.
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
(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.
Created attachment 256860 [details] patch for landing
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.
landed in: http://trac.webkit.org/changeset/186860
(In reply to comment #135) > landed in: > http://trac.webkit.org/changeset/186860 Congrats!!