[webkit-reviews] review denied: [Bug 119417] Use op_get_by_id_custom_stub to fast access to JSCallbackObject's static value : [Attachment 208682] updated with test

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Yi Shen
<max.hong.shen at gmail.com>'s request for review:
Bug 119417: Use op_get_by_id_custom_stub to fast access to JSCallbackObject's
static value
https://bugs.webkit.org/show_bug.cgi?id=119417

Attachment 208682: updated with test
https://bugs.webkit.org/attachment.cgi?id=208682&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=208682&action=review


I think this approach will work, but I'd like to see some refinement.

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:172
> +		       JSValue value = getStaticValue(exec, thisObject,
propertyName);
>		       if (value) {
> +			   if (thisObject->classRef()->isCacheable())
> +			       entry->cacheable = true;

I'd prefer to see the cacheable field initialized when the StaticValueEntry was
created. You can do this in
OpaqueJSClassContextData::OpaqueJSClassContextData().

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:189
> +    slot.setUnCacheable(); // Don't cache Parent's property

I don't think this comment adds anything that wasn't there in code.

Also, we should only do this if our class is uncacheable, right? We don't want
to make all parent class properties unconditionally uncacheable.

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:301
> +    Parent::put(thisObject, exec, propertyName, value, slot);
> +    slot.setUnCacheable(); // Don't cache Parent's property
> +    return;

Ditto.

> Source/JavaScriptCore/API/JSClassRef.h:48
> +    bool cacheable;

Let's call this "isCacheable".

> Source/JavaScriptCore/API/JSClassRef.h:127
> +    bool m_cacheable;

Let's call this "m_isCacheable".

> Source/JavaScriptCore/runtime/PropertySlot.h:64
> +    void setUnCacheable() { m_offset = invalidOffset; }

"uncacheable" is one word, so let's call this "setUncacheable".

> Source/JavaScriptCore/runtime/PutPropertySlot.h:68
> +	   void setUnCacheable() { m_type = Uncachable; }

Ditto.