Bug 74708

Created attachment 119603 [details] Patch

In-band signaling is error-prone. It's not great that you had to add an explanatory comment to every call site. I'd prefer to see some refactoring here: (1) Rename findScopedProperty to resolve; (2) Change resolve to return an object, ResolveResult, describing what it found. (3) Add "I found a block-scoped variable" as a feature to ResolveResult. (4) Add optimizations to take advantage of (3).

(In reply to comment #2) > In-band signaling is error-prone. It's not great that you had to add an explanatory comment to every call site. > > I'd prefer to see some refactoring here: > > (1) Rename findScopedProperty to resolve; > > (2) Change resolve to return an object, ResolveResult, describing what it found. > > (3) Add "I found a block-scoped variable" as a feature to ResolveResult. > > (4) Add optimizations to take advantage of (3). Thanks for the review. I will work on a new patch that does this. Incidentally, V8's implementation of block-scoped lexicals drove a similar refactor within V8. Andy

Created attachment 122960 [details] Patch

I have a new patch that includes the work from bug 74633, and significantly improves on this one, according to feedback. It happens to be based on the scopenode/codenode separation from bug 74509, which I still think is a good idea, but is not essential to this patch. More details when I upload the thing.

Created attachment 132634 [details] Patch

OK, attached the new patch. It uses a visitor to compute the block scope depth, though perhaps it could reserve temporaries instead as Oliver suggested. It is not tested enough. It passes run-javascriptcore-tests with debugging settings, but I have not tried it with the classic interpreter, or the LLInt which doesn't yet work on my platform. But, I think it is a great base for ES6 block scope. What do you think about the opcode changes? Would you prefer branching instructions for the barriers? With some work we should be able to remove some barriers in the parser, and all possible barriers in the DFG. Do we need an additional flag on symboltable entries indicating the need for r/w barriers? I'm sure there are many open questions that you still might have. I'm all ears!

Created attachment 132796 [details] Patch

Just uploaded a new patch in response to some informal review by Oliver. Changes: * Removed the nodevisitor stuff and the separate pre-pass. * Added an arg to op_reify_scope to explicitly give the base register. * Rebased on top of ToT instead of bug 74509. Still untested with the classic interpreter. Still no DFG support for the new opcodes. Still untested with LLInt, and I still haven't run all the layout tests yet. But it's a clean patch and review is welcome.

Comment on attachment 132796 [details] Patch Attachment 132796 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12005223

Comment on attachment 132796 [details] Patch Attachment 132796 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12005238

Created attachment 132810 [details] Patch

Updated patch changes: * Turns out, op_unset is the same as op_init_lazy_reg. So I removed op_unset and changed the compiler to emit init_lazy_reg instead. * Better baseline JIT code for the isEmpty / !isEmpty() checks. * Complete but untested llint implementation. Problems: * I'm pretty sure that BytecodeCompiler::resolve is doing the wrong thing in the presence of lazy unreified block scopes. It would be cleanest if we could count from the scope chain that was current when the function was entered. * The error messages you would get when falling afoul of the temporal dead zone are pretty bad. I need to figure out something better. * Still haven't run the full tests. * Probably there are some symbol table files to update for the changes in opcodes.

Attachment 132810 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2323: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 132810 [details] Patch Attachment 132810 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12005245

Comment on attachment 132810 [details] Patch Attachment 132810 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12009153

Comment on attachment 132810 [details] Patch Attachment 132810 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11994305

Comment on attachment 132810 [details] Patch Attachment 132810 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12000300

Comment on attachment 132810 [details] Patch Attachment 132810 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12000314

Created attachment 132839 [details] Patch

Changes in the new patch: * Possibly fix 32_64 builds. * Add a flag to SymbolTableEntry to indicate variables that need the temporal dead zone. Only emit the assert_lazy_var_init / assert_lazy_var_not_init opcodes for block-scoped locals that need the TDZ. To see what sort of code this produces: > try { foo; } catch (e) { e; } 28 m_instructions; 224 bytes at 0x1bcb310; 1 parameter(s); 4 callee register(s); 0 variable(s) [ 0] enter [ 1] mov r0, Undefined(@k0) [ 4] resolve_global r0, foo(@id0) [ 10] jmp 16(->26) [ 12] catch r1 [ 14] init_lazy_reg r2 [ 16] init_lazy_reg r3 [ 18] mov r2, r1 [ 21] mov r0, r2 [ 24] tear_off_scope r3 [ 26] end r0 Here r2 is for the one block-scoped identifier, "e", and r3 is the lazy register for the reified scope. In this case the block scope does not get reified, as there is no nested function expression or call to eval. > try { foo; } catch (e) { (function(s){return eval(s);}) } 32 m_instructions; 256 bytes at 0x1bd5550; 1 parameter(s); 4 callee register(s); 0 variable(s) [ 0] enter [ 1] mov r0, Undefined(@k0) [ 4] resolve_global r0, foo(@id0) [ 10] jmp 20(->30) [ 12] catch r1 [ 14] init_lazy_reg r2 [ 16] init_lazy_reg r3 [ 18] mov r2, r1 [ 21] reify_scope r3, Cell: 0x7f660d17fca0(@k1), r2 [ 25] new_func_exp r0, f0 [ 28] tear_off_scope r3 [ 30] end r0 In this case we do get a reified scope, before the new_func_exp. It is torn off afterwards. > (function () {try { foo; } catch (e) { (function(s){return eval(s);})}})() [...] 40 m_instructions; 320 bytes at 0x1bd8aa0; 1 parameter(s); 7 callee register(s); 3 variable(s) [ 0] enter [ 1] init_lazy_reg r0 [ 3] init_lazy_reg r2 [ 5] init_lazy_reg r1 [ 7] create_activation r0 [ 9] convert_this r-7 [ 11] resolve r3, foo(@id0) [ 15] jmp 20(->35) [ 17] catch r3 [ 19] init_lazy_reg r4 [ 21] init_lazy_reg r5 [ 23] mov r4, r3 [ 26] reify_scope r5, Cell: 0x7f660d17fbe0(@k0), r4 [ 30] new_func_exp r6, f0 [ 33] tear_off_scope r5 [ 35] tear_off_activation r0, r2 [ 38] ret Undefined(@k1) Here is the same thing but in function code instead of global code. We see that the function activation was created eagerly, though that is only because of the use of eval. A different example: > (function () {var a = 10; try { throw 20; } catch (b) { return (function(s){return a + b;})()}})() [...] 60 m_instructions; 480 bytes at 0x1be0760; 1 parameter(s); 15 callee register(s); 4 variable(s) [ 0] enter [ 1] init_lazy_reg r0 [ 3] init_lazy_reg r2 [ 5] init_lazy_reg r1 [ 7] mov r3, Int32: 10(@k0) [ 10] throw Int32: 20(@k1) [ 12] jmp 43(->55) [ 14] catch r4 [ 16] init_lazy_reg r5 [ 18] init_lazy_reg r6 [ 20] mov r5, r4 [ 23] create_activation r0 [ 25] reify_scope r6, Cell: 0x7f660d17f8e0(@k2), r5 [ 29] new_func_exp r7, f0 [ 32] mov r8, Undefined(@k3) [ 35] call r7, 1, 15 [ 41] op_call_put_result r7 [ 44] tear_off_scope r6 [ 46] jmp 2(->48) [ 48] tear_off_activation r0, r2 [ 51] ret r7 [ 53] tear_off_scope r6 [ 55] tear_off_activation r0, r2 [ 58] ret Undefined(@k3) Constants: k0 = Int32: 10 k1 = Int32: 20 k2 = Cell: 0x7f660d17f8e0 k3 = Undefined [...] 30 Here we see lazy reification of the activation and of the scope, and corresponding tearoff. There is no more jmp_scopes because things are getting too complicated for an opcode: better to emit what the opcode should do, directly.

Comment on attachment 132839 [details] Patch Attachment 132839 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11994360

Created attachment 133011 [details] Patch

Changes in updated patch: * Remove op_assert_lazy_reg_not_init. I was under the mistaken impression that it was permitted to assign to const variables. No write barrier is needed, because the compiler should throw an error on an attempt to mutate a read-only binding. If it goes through the generic path the runtime will see isReadOnly() on the symboltableentry. * Attempt to fix the llint. * We no longer clear registers when leaving a block scope. * An attempt to tame the combinatoric explosion of ResolveResult::type() by separating type in to type proper and attributes (e.g. read-only, needs-barrier, is-arguments, etc).

Comment on attachment 133011 [details] Patch Attachment 133011 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12035396

Comment on attachment 133011 [details] Patch Attachment 133011 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12086336

Comment on attachment 133011 [details] Patch Attachment 133011 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12034367

Created attachment 133014 [details] Patch

The last patch was b0rken. Uploading again!

Created attachment 133795 [details] Patch

The new patch gives good errors when a user tries to access an uninitialized block-scoped local. Next up is to fix the scope depth stuff, and then this patch will be fully baked. Review in advance is appreciated though :)

Created attachment 133822 [details] Patch

Latest patch avoids init_lazy_reg for block-scoped locals that don't need the TDZ.

Comment on attachment 133822 [details] Patch This looks like it needs a massive rebase.