[webkit-reviews] review granted: [Bug 185652] JSC should have InstanceOf inline caching : [Attachment 340665] the patch

[bugzilla-daemon at webkit.org]

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.