[webkit-reviews] review granted: [Bug 90982] LLInt: Add op_get_by_id_proto : [Attachment 151713] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Andy Wingo <wingo at igalia.com>'s
request for review:
Bug 90982: LLInt: Add op_get_by_id_proto
https://bugs.webkit.org/show_bug.cgi?id=90982

Attachment 151713: Patch
https://bugs.webkit.org/attachment.cgi?id=151713&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=151713&action=review


> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2049
> +		   if (!curInstruction[4].u.structure ||
Heap::isMarked(curInstruction[4].u.structure.get()))
> +		       break;

What if the main structure is marked but the prototype structure isn't?  I
guess this should be:

ASSERT(!!curInstruction[4].u.structure == !!curInstruction[5].u.structure); //
The structures should either be both 0 or both non-0.
if (!curInstruction[4].u.structure)
    break;
if (Heap::isMarked(curInstruction[4].u.structure.get()) &&
Heap::isMarked(curInstruction[5].u.structure.get()))
    break;

Though, I think it would be even better if the clearing you're doing below
flipped this back to op_get_by_id - that's the style I've been using for
put_by_id.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:508
> +_llint_op_get_by_id_proto:
> +    traceExecution()
> +    callSlowPath(_llint_slow_path_get_by_id_proto)
> +    dispatch(9)

So you get a speed-up even though this isn't inline?  Nifty.  We should inline
it at some point.