[webkit-reviews] review granted: [Bug 185652] JSC should have InstanceOf inline caching : [Attachment 340665] the patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu May 17 18:14:00 PDT 2018
Saam Barati <sbarati at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 185652: JSC should have InstanceOf inline caching
https://bugs.webkit.org/show_bug.cgi?id=185652
Attachment 340665: the patch
https://bugs.webkit.org/attachment.cgi?id=340665&action=review
--- Comment #40 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 340665
--> https://bugs.webkit.org/attachment.cgi?id=340665
the patch
View in context: https://bugs.webkit.org/attachment.cgi?id=340665&action=review
r=me
> JSTests/stress/instanceof-prototype-change.js:21
> +Bar.prototype.__proto__ = {}
Can you also add a test where you cut off the prototype to null somewhere in
the prototype chain?
Can you also add a test where you insert a prototype in a InstanceOfMiss case?
(If you don't have such a test already)
>> Source/JavaScriptCore/API/tests/testapi.mm:-1500
>> - }
>
> Let's do this in its own patch
I opened https://bugs.webkit.org/show_bug.cgi?id=185754 for this
> Source/JavaScriptCore/b3/B3Effects.h:112
> + static Effects forReadOnlyCall()
> + {
> + Effects result;
> + result.exitsSideways = true;
> + result.controlDependent = true;
> + result.reads = HeapRange::top();
> + result.readsPinned = true;
> + result.fence = true;
> + return result;
> + }
This is unused. Did you mean to use it in FTL lower?
> Source/JavaScriptCore/bytecode/AccessCase.cpp:534
> + // Legend: value = `base instanceof this`.
I actually think it'd be helpful to say something like this:
value = `a instanceof b`
where
base = a
this = b.prototype
> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:265
> +
please revert
> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:251
> +
please revert
> Source/JavaScriptCore/bytecode/PropertyCondition.cpp:195
> + if (structure->isDictionary()) {
Why do we care about this for HasPrototype? Don't we still transition
dictionaries when we set their prototype?
> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3437
> + m_jit.recordCallSiteAndGenerateExceptionHandlingOSRExitIfNeeded(
> + node->origin.semantic, m_stream->size());
I don't think we need this. We can't throw from this IC. I think you just want:
m_jit.addCallSite(CodeOrigin)
> Source/JavaScriptCore/jit/JITOperations.cpp:2204
> + bool result = JSObject::defaultHasInstance(exec, value, proto);
Do we want an exception check here?
> Source/JavaScriptCore/runtime/JSCellInlines.h:218
> + return m_type == ImpureProxyType || m_type == PureForwardingProxyType ||
m_type == ProxyObjectType;
It's a little weird this returns true for ES6 proxies. I feel like we treat
that differently than JSProxy.
More information about the webkit-reviews
mailing list