Repatch should support setters and plant calls to them directly
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 8 Apr 2014 19:39:55 +0000 (19:39 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 8 Apr 2014 19:39:55 +0000 (19:39 +0000)
https://bugs.webkit.org/show_bug.cgi?id=130750

Source/JavaScriptCore:

Reviewed by Geoffrey Garen.

All of the infrastructure was in place so this just enables setter optimization.

This is a 12x speed-up on setter microbenchmarks. This is a 1% speed-up on Octane.

* bytecode/PolymorphicPutByIdList.cpp:
(JSC::PutByIdAccess::visitWeak):
* bytecode/PolymorphicPutByIdList.h:
(JSC::PutByIdAccess::setter):
(JSC::PutByIdAccess::customSetter): Deleted.
* bytecode/PutByIdStatus.cpp:
(JSC::PutByIdStatus::computeForStubInfo):
* jit/Repatch.cpp:
(JSC::toString):
(JSC::kindFor):
(JSC::customFor):
(JSC::generateByIdStub):
(JSC::tryCachePutByID):
(JSC::tryBuildPutByIdList):
* runtime/JSObject.cpp:
(JSC::JSObject::put):
* runtime/Lookup.h:
(JSC::putEntry):
* runtime/PutPropertySlot.h:
(JSC::PutPropertySlot::setCacheableSetter):
(JSC::PutPropertySlot::isCacheableSetter):
(JSC::PutPropertySlot::isCacheableCustom):
(JSC::PutPropertySlot::setCacheableCustomProperty): Deleted.
(JSC::PutPropertySlot::isCacheableCustomProperty): Deleted.
* tests/stress/setter.js: Added.
(foo):

LayoutTests:

Reviewed by Geoffrey Garen.

* js/regress/script-tests/setter.js: Added.
* js/regress/setter-expected.txt: Added.
* js/regress/setter.html: Added.

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

13 files changed:
LayoutTests/ChangeLog
LayoutTests/js/regress/script-tests/setter.js [new file with mode: 0644]
LayoutTests/js/regress/setter-expected.txt [new file with mode: 0644]
LayoutTests/js/regress/setter.html [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/PolymorphicPutByIdList.cpp
Source/JavaScriptCore/bytecode/PolymorphicPutByIdList.h
Source/JavaScriptCore/bytecode/PutByIdStatus.cpp
Source/JavaScriptCore/jit/Repatch.cpp
Source/JavaScriptCore/runtime/JSObject.cpp
Source/JavaScriptCore/runtime/Lookup.h
Source/JavaScriptCore/runtime/PutPropertySlot.h
Source/JavaScriptCore/tests/stress/setter.js [new file with mode: 0644]

index 8438385..8343d0a 100644 (file)
@@ -1,3 +1,14 @@
+2014-04-07  Filip Pizlo  <fpizlo@apple.com>
+
+        Repatch should support setters and plant calls to them directly
+        https://bugs.webkit.org/show_bug.cgi?id=130750
+
+        Reviewed by Geoffrey Garen.
+
+        * js/regress/script-tests/setter.js: Added.
+        * js/regress/setter-expected.txt: Added.
+        * js/regress/setter.html: Added.
+
 2014-04-08  Morten Stenshorne  <mstensho@opera.com>
 
         [New Multicolumn] Child top margin sometimes ignored for column balancing
diff --git a/LayoutTests/js/regress/script-tests/setter.js b/LayoutTests/js/regress/script-tests/setter.js
new file mode 100644 (file)
index 0000000..d1eda04
--- /dev/null
@@ -0,0 +1,10 @@
+(function() {
+    var o = {_f:42};
+    o.__defineSetter__("f", function(value) { this._f = value; });
+    var n = 2000000;
+    for (var i = 0; i < n; ++i)
+        o.f = i;
+    if (o._f != n - 1)
+        throw "Error: bad result: " + o._f;
+})();
+
diff --git a/LayoutTests/js/regress/setter-expected.txt b/LayoutTests/js/regress/setter-expected.txt
new file mode 100644 (file)
index 0000000..42d39df
--- /dev/null
@@ -0,0 +1,10 @@
+JSRegress/setter
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS no exception thrown
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/js/regress/setter.html b/LayoutTests/js/regress/setter.html
new file mode 100644 (file)
index 0000000..4fcd434
--- /dev/null
@@ -0,0 +1,12 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+</head>
+<body>
+<script src="../../resources/regress-pre.js"></script>
+<script src="script-tests/setter.js"></script>
+<script src="../../resources/regress-post.js"></script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
index 0bd2588..85e908e 100644 (file)
@@ -1,5 +1,43 @@
 2014-04-07  Filip Pizlo  <fpizlo@apple.com>
 
+        Repatch should support setters and plant calls to them directly
+        https://bugs.webkit.org/show_bug.cgi?id=130750
+
+        Reviewed by Geoffrey Garen.
+        
+        All of the infrastructure was in place so this just enables setter optimization.
+        
+        This is a 12x speed-up on setter microbenchmarks. This is a 1% speed-up on Octane.
+
+        * bytecode/PolymorphicPutByIdList.cpp:
+        (JSC::PutByIdAccess::visitWeak):
+        * bytecode/PolymorphicPutByIdList.h:
+        (JSC::PutByIdAccess::setter):
+        (JSC::PutByIdAccess::customSetter): Deleted.
+        * bytecode/PutByIdStatus.cpp:
+        (JSC::PutByIdStatus::computeForStubInfo):
+        * jit/Repatch.cpp:
+        (JSC::toString):
+        (JSC::kindFor):
+        (JSC::customFor):
+        (JSC::generateByIdStub):
+        (JSC::tryCachePutByID):
+        (JSC::tryBuildPutByIdList):
+        * runtime/JSObject.cpp:
+        (JSC::JSObject::put):
+        * runtime/Lookup.h:
+        (JSC::putEntry):
+        * runtime/PutPropertySlot.h:
+        (JSC::PutPropertySlot::setCacheableSetter):
+        (JSC::PutPropertySlot::isCacheableSetter):
+        (JSC::PutPropertySlot::isCacheableCustom):
+        (JSC::PutPropertySlot::setCacheableCustomProperty): Deleted.
+        (JSC::PutPropertySlot::isCacheableCustomProperty): Deleted.
+        * tests/stress/setter.js: Added.
+        (foo):
+
+2014-04-07  Filip Pizlo  <fpizlo@apple.com>
+
         Setters are just getters that take an extra argument and don't return a value
         https://bugs.webkit.org/show_bug.cgi?id=131336
 
index 6cc056c..8605a50 100644 (file)
@@ -77,6 +77,7 @@ bool PutByIdAccess::visitWeak(RepatchBuffer& repatchBuffer) const
         if (!Heap::isMarked(m_chain.get()))
             return false;
         break;
+    case Setter:
     case CustomSetter:
         if (!Heap::isMarked(m_oldStructure.get()))
             return false;
index 691d13b..8361802 100644 (file)
@@ -47,6 +47,7 @@ public:
         Invalid,
         Transition,
         Replace,
+        Setter,
         CustomSetter
     };
     
@@ -88,17 +89,19 @@ public:
     }
 
 
-    static PutByIdAccess customSetter(
+    static PutByIdAccess setter(
         VM& vm,
         JSCell* owner,
+        AccessType accessType,
         Structure* structure,
         StructureChain* chain,
         PutPropertySlot::PutValueFunc customSetter,
         PassRefPtr<JITStubRoutine> stubRoutine)
     {
+        RELEASE_ASSERT(accessType == Setter || accessType == CustomSetter);
         PutByIdAccess result;
         result.m_oldStructure.set(vm, owner, structure);
-        result.m_type = CustomSetter;
+        result.m_type = accessType;
         if (chain)
             result.m_chain.set(vm, owner, chain);
         result.m_customSetter = customSetter;
index 2a208a2..a41b375 100644 (file)
@@ -205,6 +205,7 @@ PutByIdStatus PutByIdStatus::computeForStubInfo(const ConcurrentJITLocker&, Code
                     return PutByIdStatus(TakesSlowPath);
                 break;
             }
+            case PutByIdAccess::Setter:
             case PutByIdAccess::CustomSetter:
                 return PutByIdStatus(MakesCalls);
 
index 8e77ddd..5eb3521 100644 (file)
@@ -230,6 +230,25 @@ enum ByIdStubKind {
     CallCustomSetter
 };
 
+static const char* toString(ByIdStubKind kind)
+{
+    switch (kind) {
+    case GetValue:
+        return "GetValue";
+    case CallGetter:
+        return "CallGetter";
+    case CallCustomGetter:
+        return "CallCustomGetter";
+    case CallSetter:
+        return "CallSetter";
+    case CallCustomSetter:
+        return "CallCustomSetter";
+    default:
+        RELEASE_ASSERT_NOT_REACHED();
+        return nullptr;
+    }
+}
+
 static ByIdStubKind kindFor(const PropertySlot& slot)
 {
     if (slot.isCacheableValue())
@@ -250,13 +269,15 @@ static FunctionPtr customFor(const PropertySlot& slot)
 static ByIdStubKind kindFor(const PutPropertySlot& slot)
 {
     RELEASE_ASSERT(!slot.isCacheablePut());
-    RELEASE_ASSERT(slot.isCacheableCustomProperty());
+    if (slot.isCacheableSetter())
+        return CallSetter;
+    RELEASE_ASSERT(slot.isCacheableCustom());
     return CallCustomSetter;
 }
 
 static FunctionPtr customFor(const PutPropertySlot& slot)
 {
-    if (!slot.isCacheableCustomProperty())
+    if (!slot.isCacheableCustom())
         return FunctionPtr();
     return FunctionPtr(slot.customSetter());
 }
@@ -531,8 +552,9 @@ static void generateByIdStub(
     
     MacroAssemblerCodeRef code = FINALIZE_CODE_FOR(
         exec->codeBlock(), patchBuffer,
-        ("Get access stub for %s, return point %p",
-            toCString(*exec->codeBlock()).data(), successLabel.executableAddress()));
+        ("%s access stub for %s, return point %p",
+            toString(kind), toCString(*exec->codeBlock()).data(),
+            successLabel.executableAddress()));
     
     if (kind == CallGetter || kind == CallSetter)
         stubRoutine = adoptRef(new AccessorCallJITStubRoutine(code, *vm, std::move(callLinkInfo)));
@@ -1111,7 +1133,7 @@ static bool tryCachePutByID(ExecState* exec, JSValue baseValue, const Identifier
     Structure* structure = baseCell->structure();
     Structure* oldStructure = structure->previousID();
     
-    if (!slot.isCacheablePut() && !slot.isCacheableCustomProperty())
+    if (!slot.isCacheablePut() && !slot.isCacheableCustom() && !slot.isCacheableSetter())
         return false;
     if (!structure->propertyAccessesAreCacheable())
         return false;
@@ -1166,7 +1188,8 @@ static bool tryCachePutByID(ExecState* exec, JSValue baseValue, const Identifier
         stubInfo.initPutByIdReplace(*vm, codeBlock->ownerExecutable(), structure);
         return true;
     }
-    if (slot.isCacheableCustomProperty() && stubInfo.patch.spillMode == DontSpill) {
+    if ((slot.isCacheableCustom() || slot.isCacheableSetter())
+        && stubInfo.patch.spillMode == DontSpill) {
         RefPtr<JITStubRoutine> stubRoutine;
 
         StructureChain* prototypeChain = 0;
@@ -1189,7 +1212,10 @@ static bool tryCachePutByID(ExecState* exec, JSValue baseValue, const Identifier
             stubInfo.callReturnLocation.labelAtOffset(stubInfo.patch.deltaCallToSlowCase),
             stubRoutine);
 
-        list->addAccess(PutByIdAccess::customSetter(*vm, codeBlock->ownerExecutable(), structure, prototypeChain, slot.customSetter(), stubRoutine));
+        list->addAccess(PutByIdAccess::setter(
+            *vm, codeBlock->ownerExecutable(),
+            slot.isCacheableSetter() ? PutByIdAccess::Setter : PutByIdAccess::CustomSetter,
+            structure, prototypeChain, slot.customSetter(), stubRoutine));
 
         RepatchBuffer repatchBuffer(codeBlock);
         repatchBuffer.relink(stubInfo.callReturnLocation.jumpAtOffset(stubInfo.patch.deltaCallToJump), CodeLocationLabel(stubRoutine->code().code()));
@@ -1222,7 +1248,7 @@ static bool tryBuildPutByIdList(ExecState* exec, JSValue baseValue, const Identi
     Structure* oldStructure = structure->previousID();
     
     
-    if (!slot.isCacheablePut() && !slot.isCacheableCustomProperty())
+    if (!slot.isCacheablePut() && !slot.isCacheableCustom() && !slot.isCacheableSetter())
         return false;
 
     if (!structure->propertyAccessesAreCacheable())
@@ -1295,7 +1321,8 @@ static bool tryBuildPutByIdList(ExecState* exec, JSValue baseValue, const Identi
         return true;
     }
 
-    if (slot.isCacheableCustomProperty() && stubInfo.patch.spillMode == DontSpill) {
+    if ((slot.isCacheableCustom() || slot.isCacheableSetter())
+        && stubInfo.patch.spillMode == DontSpill) {
         RefPtr<JITStubRoutine> stubRoutine;
         StructureChain* prototypeChain = 0;
         PropertyOffset offset = slot.cachedOffset();
@@ -1317,7 +1344,10 @@ static bool tryBuildPutByIdList(ExecState* exec, JSValue baseValue, const Identi
             CodeLocationLabel(list->currentSlowPathTarget()),
             stubRoutine);
 
-        list->addAccess(PutByIdAccess::customSetter(*vm, codeBlock->ownerExecutable(), structure, prototypeChain, slot.customSetter(), stubRoutine));
+        list->addAccess(PutByIdAccess::setter(
+            *vm, codeBlock->ownerExecutable(),
+            slot.isCacheableSetter() ? PutByIdAccess::Setter : PutByIdAccess::CustomSetter,
+            structure, prototypeChain, slot.customSetter(), stubRoutine));
 
         RepatchBuffer repatchBuffer(codeBlock);
         repatchBuffer.relink(stubInfo.callReturnLocation.jumpAtOffset(stubInfo.patch.deltaCallToJump), CodeLocationLabel(stubRoutine->code().code()));
index 60cd59f..e303e79 100644 (file)
@@ -384,6 +384,8 @@ void JSObject::put(JSCell* cell, ExecState* exec, PropertyName propertyName, JSV
             JSValue gs = obj->getDirect(offset);
             if (gs.isGetterSetter()) {
                 callSetter(exec, cell, gs, value, slot.isStrictMode() ? StrictMode : NotStrictMode);
+                if (!thisObject->structure()->isDictionary())
+                    slot.setCacheableSetter(obj, offset);
                 return;
             } else
                 ASSERT(!(attributes & Accessor));
index e5233c5..0491f55 100644 (file)
@@ -270,7 +270,7 @@ namespace JSC {
                 thisObject->putDirect(exec->vm(), propertyName, value);
         } else if (!(entry->attributes() & ReadOnly)) {
             entry->propertyPutter()(exec, base, JSValue::encode(slot.thisValue()), JSValue::encode(value));
-            slot.setCacheableCustomProperty(base, entry->propertyPutter());
+            slot.setCustomProperty(base, entry->propertyPutter());
         } else if (slot.isStrictMode())
             throwTypeError(exec, StrictModeReadonlyPropertyWriteError);
     }
index 3972882..503411b 100644 (file)
@@ -38,7 +38,7 @@ namespace JSC {
     
     class PutPropertySlot {
     public:
-        enum Type { Uncachable, ExistingProperty, NewProperty, CustomProperty, CacheableCustomProperty };
+        enum Type { Uncachable, ExistingProperty, NewProperty, SetterProperty, CustomProperty };
         enum Context { UnknownContext, PutById, PutByIdEval };
         typedef void (*PutValueFunc)(ExecState*, JSObject* base, EncodedJSValue thisObject, EncodedJSValue value);
 
@@ -72,13 +72,14 @@ namespace JSC {
             m_base = base;
             m_putFunction = function;
         }
-
-        void setCacheableCustomProperty(JSObject* base, PutValueFunc function)
+        
+        void setCacheableSetter(JSObject* base, PropertyOffset offset)
         {
-            m_type = CacheableCustomProperty;
+            m_type = SetterProperty;
             m_base = base;
-            m_putFunction = function;
+            m_offset = offset;
         }
+
         PutValueFunc customSetter() const { return m_putFunction; }
 
         Context context() const { return static_cast<Context>(m_context); }
@@ -89,7 +90,8 @@ namespace JSC {
 
         bool isStrictMode() const { return m_isStrictMode; }
         bool isCacheablePut() const { return m_type == NewProperty || m_type == ExistingProperty; }
-        bool isCacheableCustomProperty() const { return m_type == CacheableCustomProperty; }
+        bool isCacheableSetter() const { return m_type == SetterProperty; }
+        bool isCacheableCustom() const { return m_type == CustomProperty; }
 
         PropertyOffset cachedOffset() const
         {
diff --git a/Source/JavaScriptCore/tests/stress/setter.js b/Source/JavaScriptCore/tests/stress/setter.js
new file mode 100644 (file)
index 0000000..c13d8d5
--- /dev/null
@@ -0,0 +1,47 @@
+function foo(o, v1) {
+    o.f = v1;
+    o.k = v1 * 33;
+}
+
+noInline(foo);
+
+for (var i = 0; i < 100; ++i) {
+    var o = {g_: 5};
+    o.__defineSetter__("f", function(value) { this.g_ += 42 * value; });
+    o.__defineSetter__("g", function(value) { this.g_ += 43 * value; });
+    o.__defineSetter__("h", function(value) { this.g_ += 44 * value; });
+    o.__defineSetter__("i", function(value) { this.g_ += 45 * value; });
+    o.__defineSetter__("j", function(value) { this.g_ += 46 * value; });
+    o.__defineSetter__("k", function(value) { this.g_ += 47 * value; });
+    foo(o, 29);
+    if (o.g_ != 5 + 42 * 29 + 29 * 47 * 33)
+        throw "Error: bad result: " + o.g_;
+}
+
+// Test the case where those fields aren't setters anymore.
+var o = {g_: 5};
+o.f = 1;
+o.g = 2;
+o.h = 3;
+o.i = 4;
+o.j = 5;
+o.k = 6;
+foo(o, 29);
+if (o.g_ != 5)
+    throw "Error: bad value of g_: " + o.g_;
+if (o.f != 29)
+    throw "Error: bad value of f: " + o.f;
+if (o.k != 29 * 33)
+    throw "Error: bad value of k: " + o.k;
+
+// Test the case where they are setters but they're not the same setters.
+var o = {g_: 5};
+o.__defineSetter__("f", function(value) { this.g_ += 52 * value; });
+o.__defineSetter__("g", function(value) { this.g_ += 53 * value; });
+o.__defineSetter__("h", function(value) { this.g_ += 54 * value; });
+o.__defineSetter__("i", function(value) { this.g_ += 55 * value; });
+o.__defineSetter__("j", function(value) { this.g_ += 56 * value; });
+o.__defineSetter__("k", function(value) { this.g_ += 57 * value; });
+foo(o, 29);
+if (o.g_ != 5 + 52 * 29 + 29 * 57 * 33)
+    throw "Error: bad result at end: " + o.g_;