[Webkit-unassigned] [Bug 162124] op_get_by_id_with_this should use inline caching

[bugzilla-daemon at webkit.org]

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

--- Comment #73 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 300268
  --> https://bugs.webkit.org/attachment.cgi?id=300268
Patch Updated

View in context: https://bugs.webkit.org/attachment.cgi?id=300268&action=review

> JSTests/stress/super-get-by-id.js:116
> +for (let i = 0; i < 1000000; i++) {
> +    try {
> +        getterValue(subObj);
> +        assert(false);
> +    } catch(e) {
> +        assert(e instanceof TypeError);
> +    };
> +}

Please add some more interesting exception tests where the exception handler has to recover values.

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1153
> +            GPRReg baseForCustomValue = m_type == CustomValueGetter || m_type == CustomValueSetter ? baseForAccessGPR : baseForCustomGetGPR;

I think the above comment makes this code wrong. We should have tests for this. Do you already have tests?
According to the above comment:
- CustomValue should be passed the property holder, which might not necessarily be |this|, however, your code says that it is.
- Custom accessors are indeed passed |this|.

Please add tests for both of these.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:261
> +    if (basePayloadGPR == resultTagGPR) {
> +        RELEASE_ASSERT(basePayloadGPR != resultPayloadGPR);
> +        
> +        if (baseTagGPROrNone == resultPayloadGPR) {
> +            m_jit.swap(basePayloadGPR, baseTagGPROrNone);
> +            baseTagGPROrNone = resultTagGPR;
> +        } else
> +            m_jit.move(basePayloadGPR, resultPayloadGPR);
> +        
> +        basePayloadGPR = resultPayloadGPR;
> +    }
> +    
> +    // Repeat the same logic for this payload and result tag.
> +    if (thisPayloadGPR == resultTagGPR) {
> +        RELEASE_ASSERT(thisPayloadGPR != resultPayloadGPR);
> +        
> +        if (thisTagGPR == resultPayloadGPR) {
> +            m_jit.swap(thisPayloadGPR, thisTagGPR);
> +            thisTagGPR = resultTagGPR;
> +        } else
> +            m_jit.move(thisPayloadGPR, resultPayloadGPR);
> +        
> +        thisPayloadGPR = resultPayloadGPR;
> +    }

This is only possible in `cachedGetById` because it uses the `Reuse` tag when allocating the result register. You don't do that here, so it should be impossible for this to ever be true.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4391
> +            base.use();
> +            thisValue.use();
> +            

I know GetById does this, but it's slightly wrong. You're saying you're done with this the resulting values of these nodes at this point, but it's not quite true.  If an exception is thrown, you'll need their values.

(The fact that GetById calls use() is something I want to fix at some point, it's somewhat arguable that any node should be doing this manually now that the DFG handles exceptions)

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2869
> +        } else if (m_node->child1().useKind() == CellUse || m_node->child2().useKind() == CellUse) {

This case should now be impossible, please remove.

> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:123
> +    UniquedStringImpl*, JSValueRegs base, JSValueRegs value, JSValueRegs thisRegs, AccessType accessType)

Style nit:
Your register ordering is a bit off with how we usually do it. The result should either be the first or the last register. It's against the style we usually have that it's in between to operands.

> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:676
> +    emitLoad(base, regT1, regT0);
> +    emitJumpSlowCaseIfNotJSCell(base, regT1);
> +    emitLoad(thisVReg, regT4, regT3);
> +    emitJumpSlowCaseIfNotJSCell(thisVReg, regT4);

This still has the problem I was talking about in my review of an earlier patch. The slow case does not handle regT3/regT4 not being populated.

> Source/JavaScriptCore/jit/Repatch.cpp:299
> +            // we don't emmit IC for DOMJIT when op is get_by_id_with_this

typo: "emmit" => "emit"

-- 
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/20170205/272ed664/attachment.html>