Indexed getters should return values directly on the PropertySlot.
authorakling@apple.com <akling@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 6 Jun 2014 23:56:50 +0000 (23:56 +0000)
committerakling@apple.com <akling@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 6 Jun 2014 23:56:50 +0000 (23:56 +0000)
<https://webkit.org/b/133586>

Source/JavaScriptCore:
Remove PropertySlot's custom index mode.

Reviewed by Darin Adler.

* runtime/JSObject.h:
(JSC::PropertySlot::getValue):
* runtime/PropertySlot.h:
(JSC::PropertySlot::setCustomIndex): Deleted.

Source/WebCore:
Make indexed getters more efficient by using PropertySlot::setValue()
to pass the value directly back through the slot, instead of giving it
a function pointer that JSC would then immediately call back through
to retrieve the value.

The function pointer approach would make sense if we did inline caching
of indexed getters but since we currently don't, this is pure overhead.

Reviewed by Darin Adler.

* bindings/js/JSCSSStyleDeclarationCustom.cpp:
(WebCore::JSCSSStyleDeclaration::getOwnPropertySlotDelegate):
(WebCore::cssPropertyGetterPixelOrPosPrefixCallback): Deleted.
(WebCore::cssPropertyGetterCallback): Deleted.
* bindings/js/JSDOMWindowCustom.cpp:
(WebCore::JSDOMWindow::getOwnPropertySlot):
(WebCore::JSDOMWindow::getOwnPropertySlotByIndex):
(WebCore::indexGetter): Deleted.
* bindings/scripts/CodeGeneratorJS.pm:
(GenerateGetOwnPropertySlotBody):
(GenerateHeader):
(GetIndexedGetterExpression):
(GenerateImplementation):
* bridge/runtime_array.cpp:
(JSC::RuntimeArray::getOwnPropertySlot):
(JSC::RuntimeArray::getOwnPropertySlotByIndex):
(JSC::RuntimeArray::indexGetter): Deleted.
* bridge/runtime_array.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@169668 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/JSObject.h
Source/JavaScriptCore/runtime/PropertySlot.h
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp
Source/WebCore/bindings/js/JSDOMWindowCustom.cpp
Source/WebCore/bindings/scripts/CodeGeneratorJS.pm
Source/WebCore/bindings/scripts/test/JS/JSTestEventTarget.cpp
Source/WebCore/bindings/scripts/test/JS/JSTestEventTarget.h
Source/WebCore/bridge/runtime_array.cpp
Source/WebCore/bridge/runtime_array.h

index ca059f1..9f8e580 100644 (file)
@@ -1,3 +1,17 @@
+2014-06-06  Andreas Kling  <akling@apple.com>
+
+        Indexed getters should return values directly on the PropertySlot.
+        <https://webkit.org/b/133586>
+
+        Remove PropertySlot's custom index mode.
+
+        Reviewed by Darin Adler.
+
+        * runtime/JSObject.h:
+        (JSC::PropertySlot::getValue):
+        * runtime/PropertySlot.h:
+        (JSC::PropertySlot::setCustomIndex): Deleted.
+
 2014-06-04  Timothy Horton  <timothy_horton@apple.com>
 
         iOS Debug build fix
index 8e98f31..ab092d7 100644 (file)
@@ -1560,8 +1560,6 @@ ALWAYS_INLINE JSValue PropertySlot::getValue(ExecState* exec, PropertyName prope
 {
     if (m_propertyType == TypeValue)
         return JSValue::decode(m_data.value);
-    if (m_propertyType == TypeCustomIndex)
-        return JSValue::decode(m_data.customIndex.getIndexValue(exec, slotBase(), JSValue::encode(m_thisValue), m_data.customIndex.index));
     if (m_propertyType == TypeGetter)
         return functionGetter(exec);
     return JSValue::decode(m_data.custom.getValue(exec, slotBase(), JSValue::encode(m_thisValue), propertyName));
@@ -1571,8 +1569,6 @@ ALWAYS_INLINE JSValue PropertySlot::getValue(ExecState* exec, unsigned propertyN
 {
     if (m_propertyType == TypeValue)
         return JSValue::decode(m_data.value);
-    if (m_propertyType == TypeCustomIndex)
-        return JSValue::decode(m_data.customIndex.getIndexValue(exec, slotBase(), JSValue::encode(m_thisValue), m_data.customIndex.index));
     if (m_propertyType == TypeGetter)
         return functionGetter(exec);
     return JSValue::decode(m_data.custom.getValue(exec, slotBase(), JSValue::encode(m_thisValue), Identifier::from(exec, propertyName)));
index 08cac7c..0345089 100644 (file)
@@ -52,8 +52,7 @@ class PropertySlot {
         TypeUnset,
         TypeValue,
         TypeGetter,
-        TypeCustom,
-        TypeCustomIndex
+        TypeCustom
     };
 
     enum CacheabilityType {
@@ -72,7 +71,6 @@ public:
     }
 
     typedef EncodedJSValue (*GetValueFunc)(ExecState*, JSObject* slotBase, EncodedJSValue thisValue, PropertyName);
-    typedef EncodedJSValue (*GetIndexValueFunc)(ExecState*, JSObject* slotBase, EncodedJSValue thisValue, unsigned);
 
     JSValue getValue(ExecState*, PropertyName) const;
     JSValue getValue(ExecState*, unsigned propertyName) const;
@@ -180,19 +178,6 @@ public:
         m_offset = !invalidOffset;
     }
 
-    void setCustomIndex(JSObject* slotBase, unsigned attributes, unsigned index, GetIndexValueFunc getIndexValue)
-    {
-        ASSERT(getIndexValue);
-        m_data.customIndex.getIndexValue = getIndexValue;
-        m_data.customIndex.index = index;
-        m_attributes = attributes;
-
-        ASSERT(slotBase);
-        m_slotBase = slotBase;
-        m_propertyType = TypeCustomIndex;
-        m_offset = invalidOffset;
-    }
-
     void setGetterSlot(JSObject* slotBase, unsigned attributes, GetterSetter* getterSetter)
     {
         ASSERT(getterSetter);
@@ -244,10 +229,6 @@ private:
         struct {
             GetValueFunc getValue;
         } custom;
-        struct {
-            GetIndexValueFunc getIndexValue;
-            unsigned index;
-        } customIndex;
     } m_data;
 
     PropertyType m_propertyType;
index 552f91c..041eec0 100644 (file)
@@ -1,3 +1,37 @@
+2014-06-06  Andreas Kling  <akling@apple.com>
+
+        Indexed getters should return values directly on the PropertySlot.
+        <https://webkit.org/b/133586>
+
+        Make indexed getters more efficient by using PropertySlot::setValue()
+        to pass the value directly back through the slot, instead of giving it
+        a function pointer that JSC would then immediately call back through
+        to retrieve the value.
+
+        The function pointer approach would make sense if we did inline caching
+        of indexed getters but since we currently don't, this is pure overhead.
+
+        Reviewed by Darin Adler.
+
+        * bindings/js/JSCSSStyleDeclarationCustom.cpp:
+        (WebCore::JSCSSStyleDeclaration::getOwnPropertySlotDelegate):
+        (WebCore::cssPropertyGetterPixelOrPosPrefixCallback): Deleted.
+        (WebCore::cssPropertyGetterCallback): Deleted.
+        * bindings/js/JSDOMWindowCustom.cpp:
+        (WebCore::JSDOMWindow::getOwnPropertySlot):
+        (WebCore::JSDOMWindow::getOwnPropertySlotByIndex):
+        (WebCore::indexGetter): Deleted.
+        * bindings/scripts/CodeGeneratorJS.pm:
+        (GenerateGetOwnPropertySlotBody):
+        (GenerateHeader):
+        (GetIndexedGetterExpression):
+        (GenerateImplementation):
+        * bridge/runtime_array.cpp:
+        (JSC::RuntimeArray::getOwnPropertySlot):
+        (JSC::RuntimeArray::getOwnPropertySlotByIndex):
+        (JSC::RuntimeArray::indexGetter): Deleted.
+        * bridge/runtime_array.h:
+
 2014-06-06  Brent Fulgham  <bfulgham@apple.com>
 
         GenericCueData elements prematurely removed
index 94577c2..11de5d3 100644 (file)
@@ -297,14 +297,6 @@ static inline JSValue cssPropertyGetterPixelOrPosPrefix(ExecState* exec, JSCSSSt
     return getPropertyValueFallback(exec, thisObj, propertyID);
 }
 
-static EncodedJSValue cssPropertyGetterPixelOrPosPrefixCallback(ExecState* exec, JSObject*, EncodedJSValue thisValue, unsigned propertyID)
-{
-    JSCSSStyleDeclaration* thisObject = jsDynamicCast<JSCSSStyleDeclaration*>(JSValue::decode(thisValue));
-    if (!thisObject)
-        return throwVMTypeError(exec);
-    return JSValue::encode(cssPropertyGetterPixelOrPosPrefix(exec, thisObject, propertyID));
-}
-
 static inline JSValue cssPropertyGetter(ExecState* exec, JSCSSStyleDeclaration* thisObj, unsigned propertyID)
 {
     RefPtr<CSSValue> v = thisObj->impl().getPropertyCSSValueInternal(static_cast<CSSPropertyID>(propertyID));
@@ -314,24 +306,16 @@ static inline JSValue cssPropertyGetter(ExecState* exec, JSCSSStyleDeclaration*
     return getPropertyValueFallback(exec, thisObj, propertyID);
 }
 
-static EncodedJSValue cssPropertyGetterCallback(ExecState* exec, JSObject*, EncodedJSValue thisValue, unsigned propertyID)
-{
-    JSCSSStyleDeclaration* thisObject = jsDynamicCast<JSCSSStyleDeclaration*>(JSValue::decode(thisValue));
-    if (!thisObject)
-        return throwVMTypeError(exec);
-    return JSValue::encode(cssPropertyGetter(exec, thisObject, propertyID));
-}
-
-bool JSCSSStyleDeclaration::getOwnPropertySlotDelegate(ExecState*, PropertyName propertyIdentifier, PropertySlot& slot)
+bool JSCSSStyleDeclaration::getOwnPropertySlotDelegate(ExecState* exec, PropertyName propertyIdentifier, PropertySlot& slot)
 {
     CSSPropertyInfo propertyInfo = cssPropertyIDForJSCSSPropertyName(propertyIdentifier);
     if (!propertyInfo.propertyID)
         return false;
 
     if (propertyInfo.hadPixelOrPosPrefix)
-        slot.setCustomIndex(this, DontDelete, static_cast<unsigned>(propertyInfo.propertyID), cssPropertyGetterPixelOrPosPrefixCallback);
+        slot.setValue(this, DontDelete, cssPropertyGetterPixelOrPosPrefix(exec, this, propertyInfo.propertyID));
     else
-        slot.setCustomIndex(this, DontDelete, static_cast<unsigned>(propertyInfo.propertyID), cssPropertyGetterCallback);
+        slot.setValue(this, DontDelete, cssPropertyGetter(exec, this, propertyInfo.propertyID));
     return true;
 }
 
index 5b20a31..4e956f9 100644 (file)
@@ -75,11 +75,6 @@ static EncodedJSValue childFrameGetter(ExecState* exec, JSObject* slotBase, Enco
     return JSValue::encode(toJS(exec, jsCast<JSDOMWindow*>(slotBase)->impl().frame()->tree().scopedChild(propertyNameToAtomicString(propertyName))->document()->domWindow()));
 }
 
-static EncodedJSValue indexGetter(ExecState* exec, JSObject* slotBase, EncodedJSValue, unsigned index)
-{
-    return JSValue::encode(toJS(exec, jsCast<JSDOMWindow*>(slotBase)->impl().frame()->tree().scopedChild(index)->document()->domWindow()));
-}
-
 static EncodedJSValue namedItemGetter(ExecState* exec, JSObject* slotBase, EncodedJSValue, PropertyName propertyName)
 {
     JSDOMWindowBase* thisObj = jsCast<JSDOMWindow*>(slotBase);
@@ -232,7 +227,8 @@ bool JSDOMWindow::getOwnPropertySlot(JSObject* object, ExecState* exec, Property
     unsigned i = propertyName.asIndex();
     if (i < thisObject->impl().frame()->tree().scopedChildCount()) {
         ASSERT(i != PropertyName::NotAnIndex);
-        slot.setCustomIndex(thisObject, ReadOnly | DontDelete | DontEnum, i, indexGetter);
+        slot.setValue(thisObject, ReadOnly | DontDelete | DontEnum,
+            toJS(exec, thisObject->impl().frame()->tree().scopedChild(i)->document()->domWindow()));
         return true;
     }
 
@@ -308,7 +304,8 @@ bool JSDOMWindow::getOwnPropertySlotByIndex(JSObject* object, ExecState* exec, u
     // allow window[1] or parent[1] etc. (#56983)
     if (index < thisObject->impl().frame()->tree().scopedChildCount()) {
         ASSERT(index != PropertyName::NotAnIndex);
-        slot.setCustomIndex(thisObject, ReadOnly | DontDelete | DontEnum, index, indexGetter);
+        slot.setValue(thisObject, ReadOnly | DontDelete | DontEnum,
+            toJS(exec, thisObject->impl().frame()->tree().scopedChild(index)->document()->domWindow()));
         return true;
     }
 
index b26d711..1d7daca 100644 (file)
@@ -432,7 +432,7 @@ sub GenerateGetOwnPropertySlotBody
         } else {
             push(@getOwnPropertySlotImpl, "        unsigned attributes = ${namespaceMaybe}DontDelete | ${namespaceMaybe}ReadOnly;\n");
         }
-        push(@getOwnPropertySlotImpl, "        slot.setCustomIndex(thisObject, attributes, index, indexGetter);\n");
+        push(@getOwnPropertySlotImpl, "        slot.setValue(thisObject, attributes, " . GetIndexedGetterExpression($indexedGetterFunction) . ");\n");
         push(@getOwnPropertySlotImpl, "        return true;\n");
         push(@getOwnPropertySlotImpl, "    }\n");
     }
@@ -1126,11 +1126,6 @@ sub GenerateHeader
     }
     push(@headerContent, "Base::StructureFlags;\n");
 
-    # Index getter
-    if ($indexedGetterFunction) {
-        push(@headerContent, "    static JSC::EncodedJSValue indexGetter(JSC::ExecState*, JSC::JSObject*, JSC::EncodedJSValue, unsigned);\n");
-    }
-
     # Index setter
     if ($interface->extendedAttributes->{"CustomIndexedSetter"}) {
         push(@headerContent, "    void indexSetter(JSC::ExecState*, unsigned index, JSC::JSValue);\n");
@@ -1721,6 +1716,15 @@ sub GetCastingHelperForBaseObject
     return "jsCast<JS" . $interface->name . "*>";
 }
 
+sub GetIndexedGetterExpression
+{
+    my $indexedGetterFunction = shift;
+    if ($indexedGetterFunction->signature->type eq "DOMString") {
+        return "jsStringOrUndefined(exec, thisObject->impl().item(index))";
+    }
+    return "toJS(exec, thisObject->globalObject(), thisObject->impl().item(index))";
+}
+
 sub GenerateImplementation
 {
     my ($object, $interface) = @_;
@@ -2107,7 +2111,7 @@ sub GenerateImplementation
                 } else {
                     push(@implContent, "        unsigned attributes = DontDelete | ReadOnly;\n");
                 }
-                push(@implContent, "        slot.setCustomIndex(thisObject, attributes, index, thisObject->indexGetter);\n");
+                push(@implContent, "        slot.setValue(thisObject, attributes, " . GetIndexedGetterExpression($indexedGetterFunction) . ");\n");
                 push(@implContent, "        return true;\n");
                 push(@implContent, "    }\n");
             }
@@ -2889,17 +2893,9 @@ sub GenerateImplementation
     }
 
     if ($indexedGetterFunction) {
-        push(@implContent, "\nEncodedJSValue ${className}::indexGetter(ExecState* exec, JSObject* slotBase, EncodedJSValue, unsigned index)\n");
-        push(@implContent, "{\n");
-        push(@implContent, "    ${className}* thisObj = jsCast<$className*>(slotBase);\n");
-        push(@implContent, "    ASSERT_GC_OBJECT_INHERITS(thisObj, info());\n");
         if ($indexedGetterFunction->signature->type eq "DOMString") {
             $implIncludes{"URL.h"} = 1;
-            push(@implContent, "    return JSValue::encode(jsStringOrUndefined(exec, thisObj->impl().item(index)));\n");
-        } else {
-            push(@implContent, "    return JSValue::encode(toJS(exec, thisObj->globalObject(), thisObj->impl().item(index)));\n");
         }
-        push(@implContent, "}\n\n");
         if ($interfaceName =~ /^HTML\w*Collection$/ or $interfaceName eq "RadioNodeList") {
             $implIncludes{"JSNode.h"} = 1;
             $implIncludes{"Node.h"} = 1;
index debee97..4db590e 100644 (file)
@@ -157,7 +157,7 @@ bool JSTestEventTarget::getOwnPropertySlot(JSObject* object, ExecState* exec, Pr
     unsigned index = propertyName.asIndex();
     if (index != PropertyName::NotAnIndex && index < thisObject->impl().length()) {
         unsigned attributes = DontDelete | ReadOnly;
-        slot.setCustomIndex(thisObject, attributes, index, indexGetter);
+        slot.setValue(thisObject, attributes, toJS(exec, thisObject->globalObject(), thisObject->impl().item(index)));
         return true;
     }
     if (canGetItemsForName(exec, &thisObject->impl(), propertyName)) {
@@ -173,7 +173,7 @@ bool JSTestEventTarget::getOwnPropertySlotByIndex(JSObject* object, ExecState* e
     ASSERT_GC_OBJECT_INHERITS(thisObject, info());
     if (index < thisObject->impl().length()) {
         unsigned attributes = DontDelete | ReadOnly;
-        slot.setCustomIndex(thisObject, attributes, index, thisObject->indexGetter);
+        slot.setValue(thisObject, attributes, toJS(exec, thisObject->globalObject(), thisObject->impl().item(index)));
         return true;
     }
     PropertyName propertyName = Identifier::from(exec, index);
@@ -287,14 +287,6 @@ void JSTestEventTarget::visitChildren(JSCell* cell, SlotVisitor& visitor)
     thisObject->impl().visitJSEventListeners(visitor);
 }
 
-
-EncodedJSValue JSTestEventTarget::indexGetter(ExecState* exec, JSObject* slotBase, EncodedJSValue, unsigned index)
-{
-    JSTestEventTarget* thisObj = jsCast<JSTestEventTarget*>(slotBase);
-    ASSERT_GC_OBJECT_INHERITS(thisObj, info());
-    return JSValue::encode(toJS(exec, thisObj->globalObject(), thisObj->impl().item(index)));
-}
-
 bool JSTestEventTargetOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor)
 {
     JSTestEventTarget* jsTestEventTarget = jsCast<JSTestEventTarget*>(handle.slot()->asCell());
index e1edc29..22e6950 100644 (file)
@@ -79,7 +79,6 @@ protected:
     }
 
     static const unsigned StructureFlags = JSC::HasImpureGetOwnPropertySlot | JSC::InterceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero | JSC::MasqueradesAsUndefined | JSC::OverridesGetOwnPropertySlot | JSC::OverridesGetPropertyNames | JSC::OverridesVisitChildren | Base::StructureFlags;
-    static JSC::EncodedJSValue indexGetter(JSC::ExecState*, JSC::JSObject*, JSC::EncodedJSValue, unsigned);
 private:
     static bool canGetItemsForName(JSC::ExecState*, TestEventTarget*, JSC::PropertyName);
     static JSC::EncodedJSValue nameGetter(JSC::ExecState*, JSC::JSObject*, JSC::EncodedJSValue, JSC::PropertyName);
index 67214f8..d8ff9e2 100644 (file)
@@ -68,12 +68,6 @@ EncodedJSValue RuntimeArray::lengthGetter(ExecState* exec, JSObject*, EncodedJSV
     return JSValue::encode(jsNumber(thisObject->getLength()));
 }
 
-EncodedJSValue RuntimeArray::indexGetter(ExecState* exec, JSObject* slotBase, EncodedJSValue, unsigned index)
-{
-    RuntimeArray* thisObj = jsCast<RuntimeArray*>(slotBase);
-    return JSValue::encode(thisObj->getConcreteArray()->valueAt(exec, index));
-}
-
 void RuntimeArray::getOwnPropertyNames(JSObject* object, ExecState* exec, PropertyNameArray& propertyNames, EnumerationMode mode)
 {
     RuntimeArray* thisObject = jsCast<RuntimeArray*>(object);
@@ -98,7 +92,8 @@ bool RuntimeArray::getOwnPropertySlot(JSObject* object, ExecState* exec, Propert
     unsigned index = propertyName.asIndex();
     if (index < thisObject->getLength()) {
         ASSERT(index != PropertyName::NotAnIndex);
-        slot.setCustomIndex(thisObject, DontDelete | DontEnum, index, thisObject->indexGetter);
+        slot.setValue(thisObject, DontDelete | DontEnum,
+            thisObject->getConcreteArray()->valueAt(exec, index));
         return true;
     }
     
@@ -109,7 +104,8 @@ bool RuntimeArray::getOwnPropertySlotByIndex(JSObject* object, ExecState *exec,
 {
     RuntimeArray* thisObject = jsCast<RuntimeArray*>(object);
     if (index < thisObject->getLength()) {
-        slot.setCustomIndex(thisObject, DontDelete | DontEnum, index, thisObject->indexGetter);
+        slot.setValue(thisObject, DontDelete | DontEnum,
+            thisObject->getConcreteArray()->valueAt(exec, index));
         return true;
     }
     
index 998db53..b6d1f68 100644 (file)
@@ -85,7 +85,6 @@ protected:
 private:
     RuntimeArray(ExecState*, Structure*);
     static EncodedJSValue lengthGetter(ExecState*, JSObject*, EncodedJSValue, PropertyName);
-    static EncodedJSValue indexGetter(ExecState*, JSObject*, EncodedJSValue, unsigned);
 
     BindingsArray* m_array;
 };