[Webkit-unassigned] [Bug 151596] [JSC] add missing RequireObjectCoercible() step in destructuring

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=151596

--- Comment #6 from Geoffrey Garen <ggaren at apple.com> ---
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3853
> > +    emitJumpIfFalse(emitUnaryOp(op_eq_null, newTemporary(), value), target.get());
> 
> Unfortunately this emits the “masquerades as null” checking code, so it’s
> unnecessarily slow. Here we are not literally checking == null so we don’t
> want that extra logic; it just slows us down. One way to do this would be to
> add a new op_is_undefined_or_null that doesn’t bother with the masquerading
> logic. Or op_is_object_coercible.

I hate to start a debate in patch review, but let me suggestion an alternative approach: Just use op_jneq_null. (Note the "eq null" covers null and undefined, as the == operator does. So, I'm suggested a shared error message along the lines of "is not a valid destructuring parameter".)

It's true that jneq_null / jeq_null includes a masquerading check in the interpreter and first-level JIT. However, that check is reasonably cheap, and it disappears to (literally) nothing in the optimizing compilers.

The benefit of reusing the existing opcode is that it's less code to maintain, and it's fewer bytecode dispatch operations in the interpreter.

As an example of maintenance, I believe these new opcodes are not listed in FTLCapabilities.cpp, and so their use will prohibit entering the FTL. (Perhaps I missed something? But I don't see FTLCapabilities.cpp in the diff.)

Perhaps I am being overly pessimistic, and we should introduce new opcodes when it suits us.

Either way, I think it is best not to emit runtime control flow to decide which error message to print. That control flow has a cost in memory and compile time. If a developer wants to know more about the value when things go wrong, we'll automatically stop in the debugger, and they can find out.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151201/d4b8dde2/attachment.html>