<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - op_get_by_id_with_this should use inline caching"
href="https://bugs.webkit.org/show_bug.cgi?id=162124#c73">Comment # 73</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - op_get_by_id_with_this should use inline caching"
href="https://bugs.webkit.org/show_bug.cgi?id=162124">bug 162124</a>
from <span class="vcard"><a class="email" href="mailto:sbarati@apple.com" title="Saam Barati <sbarati@apple.com>"> <span class="fn">Saam Barati</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=300268&action=diff" name="attach_300268" title="Patch Updated">attachment 300268</a> <a href="attachment.cgi?id=300268&action=edit" title="Patch Updated">[details]</a></span>
Patch Updated
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=300268&action=review">https://bugs.webkit.org/attachment.cgi?id=300268&action=review</a>
<span class="quote">> 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);
> + };
> +}</span >
Please add some more interesting exception tests where the exception handler has to recover values.
<span class="quote">> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:1153
> + GPRReg baseForCustomValue = m_type == CustomValueGetter || m_type == CustomValueSetter ? baseForAccessGPR : baseForCustomGetGPR;</span >
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.
<span class="quote">> 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;
> + }</span >
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.
<span class="quote">> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4391
> + base.use();
> + thisValue.use();
> + </span >
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)
<span class="quote">> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2869
> + } else if (m_node->child1().useKind() == CellUse || m_node->child2().useKind() == CellUse) {</span >
This case should now be impossible, please remove.
<span class="quote">> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:123
> + UniquedStringImpl*, JSValueRegs base, JSValueRegs value, JSValueRegs thisRegs, AccessType accessType)</span >
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.
<span class="quote">> Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp:676
> + emitLoad(base, regT1, regT0);
> + emitJumpSlowCaseIfNotJSCell(base, regT1);
> + emitLoad(thisVReg, regT4, regT3);
> + emitJumpSlowCaseIfNotJSCell(thisVReg, regT4);</span >
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.
<span class="quote">> Source/JavaScriptCore/jit/Repatch.cpp:299
> + // we don't emmit IC for DOMJIT when op is get_by_id_with_this</span >
typo: "emmit" => "emit"</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>