Attachment 339666 Details for Bug 185348 – Patch

[patch] Patch

bug-185348-20180506123332.patch (text/plain), 17.36 KB, created by Yusuke Suzuki on 2018-05-05 20:33:33 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2018-05-05 20:33:33 PDT

Size: 17.36 KB

patch

obsolete

>Subversion Revision: 231396
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index e61aafe70d4e23936e525f18ce92bafdb48c2b50..f7efabb600e6371589901d5834834d32b408e179 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,42 @@
>+2018-05-05  Yusuke Suzuki  <utatane.tea@gmail.com>
>+
>+        [JSC] Object.assign for final objects should be faster
>+        https://bugs.webkit.org/show_bug.cgi?id=185348
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Object.assign is so heavily used to clone an object. For example, speedometer react-redux can be significantly
>+        improved if Object.assign becomes fast. It is worth adding a complex fast path to accelerate the major use cases.
>+
>+        If enumerating properties of source objects and putting properties to target object are non observable,
>+        we can avoid hash table looking up of source object properties. We can enumerate object property entries,
>+        and put them to target object. This patch adds this fast path to Object.assign implementation.
>+
>+        When enumerating properties, we need to ensure that the given |source| object does not include "__proto__"
>+        property since we cannot perform fast [[Put]] for the |target| object. We add a new flag
>+        "HasUnderscoreProtoPropertyExcludingOriginalProto" to Structure to track this state.
>+
>+        This improves object-assign.es6 by 1.85x.
>+
>+                                        baseline                  patched
>+
>+            object-assign.es6      368.6132+-8.3508     ^    198.8775+-4.9042        ^ definitely 1.8535x faster
>+
>+        And Speedometer2.0 React-Redux-TodoMVC's total time is improved from 490ms to 431ms.
>+
>+        * runtime/JSObject.h:
>+        * runtime/JSObjectInlines.h:
>+        (JSC::JSObject::canPerformFastPutInlineExcludingProto):
>+        (JSC::JSObject::canPerformFastPutInline):
>+        * runtime/ObjectConstructor.cpp:
>+        (JSC::objectConstructorAssign):
>+        * runtime/Structure.cpp:
>+        (JSC::Structure::Structure):
>+        * runtime/Structure.h:
>+        * runtime/StructureInlines.h:
>+        (JSC::Structure::forEachProperty):
>+        (JSC::Structure::add):
>+
> 2018-05-04  Keith Miller  <keith_miller@apple.com>
> 
>         isCacheableArrayLength should return true for undecided arrays
>diff --git a/Source/JavaScriptCore/runtime/JSObject.h b/Source/JavaScriptCore/runtime/JSObject.h
>index c2eb8e6936429f4f6acef77754bf636d33ea3674..e4e9b32ba083d1d98081bccbec68d1df7ab41f85 100644
>--- a/Source/JavaScriptCore/runtime/JSObject.h
>+++ b/Source/JavaScriptCore/runtime/JSObject.h
>@@ -872,6 +872,9 @@ class JSObject : public JSCell {
> 
>     JS_EXPORT_PRIVATE JSValue getMethod(ExecState*, CallData&, CallType&, const Identifier&, const String& errorMessage);
> 
>+    bool canPerformFastPutInline(VM&, PropertyName);
>+    bool canPerformFastPutInlineExcludingProto(VM&);
>+
>     DECLARE_EXPORT_INFO;
> 
> protected:
>@@ -1017,7 +1020,6 @@ class JSObject : public JSCell {
>         
>     template<PutMode>
>     bool putDirectInternal(VM&, PropertyName, JSValue, unsigned attr, PutPropertySlot&);
>-    bool canPerformFastPutInline(VM&, PropertyName);
> 
>     JS_EXPORT_PRIVATE NEVER_INLINE bool putInlineSlow(ExecState*, PropertyName, JSValue, PutPropertySlot&);
> 
>diff --git a/Source/JavaScriptCore/runtime/JSObjectInlines.h b/Source/JavaScriptCore/runtime/JSObjectInlines.h
>index 5e2701f17a433e32d1bc6e9c87a4a5589d4f3f8f..9c720a8b126b80f47b6633e4546235897dd7796b 100644
>--- a/Source/JavaScriptCore/runtime/JSObjectInlines.h
>+++ b/Source/JavaScriptCore/runtime/JSObjectInlines.h
>@@ -60,11 +60,8 @@ void createListFromArrayLike(ExecState* exec, JSValue arrayLikeValue, RuntimeTyp
>     }
> }
> 
>-ALWAYS_INLINE bool JSObject::canPerformFastPutInline(VM& vm, PropertyName propertyName)
>+ALWAYS_INLINE bool JSObject::canPerformFastPutInlineExcludingProto(VM& vm)
> {
>-    if (UNLIKELY(propertyName == vm.propertyNames->underscoreProto))
>-        return false;
>-
>     // Check if there are any setters or getters in the prototype chain
>     JSValue prototype;
>     JSObject* obj = this;
>@@ -83,6 +80,13 @@ ALWAYS_INLINE bool JSObject::canPerformFastPutInline(VM& vm, PropertyName proper
>     ASSERT_NOT_REACHED();
> }
> 
>+ALWAYS_INLINE bool JSObject::canPerformFastPutInline(VM& vm, PropertyName propertyName)
>+{
>+    if (UNLIKELY(propertyName == vm.propertyNames->underscoreProto))
>+        return false;
>+    return canPerformFastPutInlineExcludingProto(vm);
>+}
>+
> template<typename CallbackWhenNoException>
> ALWAYS_INLINE typename std::result_of<CallbackWhenNoException(bool, PropertySlot&)>::type JSObject::getPropertySlot(ExecState* exec, PropertyName propertyName, CallbackWhenNoException callback) const
> {
>diff --git a/Source/JavaScriptCore/runtime/ObjectConstructor.cpp b/Source/JavaScriptCore/runtime/ObjectConstructor.cpp
>index e18c765473a024d33bf50ced0be9c1cc1a29b166..e3f6e386a5d0a3b503d7b2b7086b811760fbc628 100644
>--- a/Source/JavaScriptCore/runtime/ObjectConstructor.cpp
>+++ b/Source/JavaScriptCore/runtime/ObjectConstructor.cpp
>@@ -300,6 +300,9 @@ EncodedJSValue JSC_HOST_CALL objectConstructorAssign(ExecState* exec)
>     JSObject* target = targetValue.toObject(exec);
>     RETURN_IF_EXCEPTION(scope, { });
> 
>+    // FIXME: Extend this for non JSFinalObject. For example, we would like to use this fast path for function objects too.
>+    bool targetCanPerformFastPut = jsDynamicCast<JSFinalObject*>(vm, target) && target->canPerformFastPutInlineExcludingProto(vm);
>+
>     unsigned argsCount = exec->argumentCount();
>     for (unsigned i = 1; i < argsCount; ++i) {
>         JSValue sourceValue = exec->uncheckedArgument(i);
>@@ -308,6 +311,55 @@ EncodedJSValue JSC_HOST_CALL objectConstructorAssign(ExecState* exec)
>         JSObject* source = sourceValue.toObject(exec);
>         RETURN_IF_EXCEPTION(scope, { });
> 
>+        // FIXME: Extend this for non JSFinalObject. For example, we would like to use this fast path for function objects too.
>+        // https://bugs.webkit.org/show_bug.cgi?id=185358
>+        if (targetCanPerformFastPut) {
>+            if (!source->staticPropertiesReified()) {
>+                source->reifyAllStaticProperties(exec);
>+                RETURN_IF_EXCEPTION(scope, { });
>+            }
>+
>+            auto canPerformFastPropertyEnumerationForObjectAssign = [] (Structure* structure) {
>+                if (structure->typeInfo().overridesGetOwnPropertySlot())
>+                    return false;
>+                if (structure->typeInfo().overridesGetPropertyNames())
>+                    return false;
>+                // FIXME: Indexed properties can be handled.
>+                // https://bugs.webkit.org/show_bug.cgi?id=185358
>+                if (hasIndexedProperties(structure->indexingType()))
>+                    return false;
>+                if (structure->hasGetterSetterProperties())
>+                    return false;
>+                if (structure->isUncacheableDictionary())
>+                    return false;
>+                // Cannot perform fast [[Put]] to |target| if the property names of the |source| contain "__proto__".
>+                if (structure->hasUnderscoreProtoPropertyExcludingOriginalProto())
>+                    return false;
>+                return true;
>+            };
>+
>+            Structure* structure = source->structure(vm);
>+            if (canPerformFastPropertyEnumerationForObjectAssign(structure)) {
>+                // |source| Structure does not have any getters. And target can perform fast put.
>+                // So enumerating properties and putting properties are non observable.
>+                structure->forEachProperty(vm, [&] (const PropertyMapEntry& entry) -> bool {
>+                    if (entry.attributes & PropertyAttribute::DontEnum)
>+                        return true;
>+
>+                    // FIXME: We could put properties in a batching manner to accelerate Object.assign more.
>+                    // https://bugs.webkit.org/show_bug.cgi?id=185358
>+                    PutPropertySlot putPropertySlot(target, true);
>+                    target->putOwnDataProperty(vm, entry.key, source->getDirect(entry.offset), putPropertySlot);
>+                    return true;
>+                });
>+                continue;
>+            }
>+        }
>+
>+        // [[GetOwnPropertyNames]], [[Get]] etc. could modify target object and invalidate this assumption.
>+        // For example, [[Get]] of source object could configure setter to target object. So disable the fast path.
>+        targetCanPerformFastPut = false;
>+
>         PropertyNameArray properties(&vm, PropertyNameMode::StringsAndSymbols, PrivateSymbolMode::Exclude);
>         source->methodTable(vm)->getOwnPropertyNames(source, exec, properties, EnumerationMode(DontEnumPropertiesMode::Include));
>         RETURN_IF_EXCEPTION(scope, { });
>diff --git a/Source/JavaScriptCore/runtime/Structure.cpp b/Source/JavaScriptCore/runtime/Structure.cpp
>index 720aff7966c9f2ee3a2c9e4d217c3214eb417491..c557e2aba33b38cc3c39ea61d5ddd7a4ee52a1ac 100644
>--- a/Source/JavaScriptCore/runtime/Structure.cpp
>+++ b/Source/JavaScriptCore/runtime/Structure.cpp
>@@ -196,6 +196,7 @@ Structure::Structure(VM& vm, JSGlobalObject* globalObject, JSValue prototype, co
>     setHasGetterSetterProperties(classInfo->hasStaticSetterOrReadonlyProperties());
>     setHasCustomGetterSetterProperties(false);
>     setHasReadOnlyOrGetterSetterPropertiesExcludingProto(classInfo->hasStaticSetterOrReadonlyProperties());
>+    setHasUnderscoreProtoPropertyExcludingOriginalProto(false);
>     setIsQuickPropertyAccessAllowedForEnumeration(true);
>     setAttributesInPrevious(0);
>     setDidPreventExtensions(false);
>@@ -229,6 +230,7 @@ Structure::Structure(VM& vm)
>     setHasGetterSetterProperties(m_classInfo->hasStaticSetterOrReadonlyProperties());
>     setHasCustomGetterSetterProperties(false);
>     setHasReadOnlyOrGetterSetterPropertiesExcludingProto(m_classInfo->hasStaticSetterOrReadonlyProperties());
>+    setHasUnderscoreProtoPropertyExcludingOriginalProto(false);
>     setIsQuickPropertyAccessAllowedForEnumeration(true);
>     setAttributesInPrevious(0);
>     setDidPreventExtensions(false);
>@@ -262,6 +264,7 @@ Structure::Structure(VM& vm, Structure* previous, DeferredStructureTransitionWat
>     setHasGetterSetterProperties(previous->hasGetterSetterProperties());
>     setHasCustomGetterSetterProperties(previous->hasCustomGetterSetterProperties());
>     setHasReadOnlyOrGetterSetterPropertiesExcludingProto(previous->hasReadOnlyOrGetterSetterPropertiesExcludingProto());
>+    setHasUnderscoreProtoPropertyExcludingOriginalProto(previous->hasUnderscoreProtoPropertyExcludingOriginalProto());
>     setIsQuickPropertyAccessAllowedForEnumeration(previous->isQuickPropertyAccessAllowedForEnumeration());
>     setAttributesInPrevious(0);
>     setDidPreventExtensions(previous->didPreventExtensions());
>diff --git a/Source/JavaScriptCore/runtime/Structure.h b/Source/JavaScriptCore/runtime/Structure.h
>index 37696bb1d039c1be24507da409863a2b9bbe5805..474106c03fa78cd87a97a7b43e76517c49919086 100644
>--- a/Source/JavaScriptCore/runtime/Structure.h
>+++ b/Source/JavaScriptCore/runtime/Structure.h
>@@ -430,6 +430,9 @@ class Structure final : public JSCell {
>     // to continue or false if it's done.
>     template<typename Functor>
>     void forEachPropertyConcurrently(const Functor&);
>+
>+    template<typename Functor>
>+    void forEachProperty(VM&, const Functor&);
>     
>     PropertyOffset getConcurrently(UniquedStringImpl* uid);
>     PropertyOffset getConcurrently(UniquedStringImpl* uid, unsigned& attributes);
>@@ -677,6 +680,7 @@ class Structure final : public JSCell {
>     DEFINE_BITFIELD(bool, transitionWatchpointIsLikelyToBeFired, TransitionWatchpointIsLikelyToBeFired, 1, 26);
>     DEFINE_BITFIELD(bool, hasBeenDictionary, HasBeenDictionary, 1, 27);
>     DEFINE_BITFIELD(bool, isAddingPropertyForTransition, IsAddingPropertyForTransition, 1, 28);
>+    DEFINE_BITFIELD(bool, hasUnderscoreProtoPropertyExcludingOriginalProto, HasUnderscoreProtoPropertyExcludingOriginalProto, 1, 29);
> 
> private:
>     friend class LLIntOffsetsExtractor;
>diff --git a/Source/JavaScriptCore/runtime/StructureInlines.h b/Source/JavaScriptCore/runtime/StructureInlines.h
>index 9047f2264bf5bcd78cf53d8ef6218c3b503a95ba..8cf28a89f1851067f5c191dc8d20eb402d5d1f51 100644
>--- a/Source/JavaScriptCore/runtime/StructureInlines.h
>+++ b/Source/JavaScriptCore/runtime/StructureInlines.h
>@@ -163,6 +163,17 @@ void Structure::forEachPropertyConcurrently(const Functor& functor)
>     }
> }
> 
>+template<typename Functor>
>+void Structure::forEachProperty(VM& vm, const Functor& functor)
>+{
>+    if (PropertyTable* table = ensurePropertyTableIfNotEmpty(vm)) {
>+        for (auto& entry : *table) {
>+            if (!functor(entry))
>+                return;
>+        }
>+    }
>+}
>+
> inline PropertyOffset Structure::getConcurrently(UniquedStringImpl* uid)
> {
>     unsigned attributesIgnored;
>@@ -376,6 +387,8 @@ inline PropertyOffset Structure::add(VM& vm, PropertyName propertyName, unsigned
>     checkConsistency();
>     if (attributes & PropertyAttribute::DontEnum || propertyName.isSymbol())
>         setIsQuickPropertyAccessAllowedForEnumeration(false);
>+    if (propertyName == vm.propertyNames->underscoreProto)
>+        setHasUnderscoreProtoPropertyExcludingOriginalProto(true);
> 
>     auto rep = propertyName.uid();
> 
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 3cc1a48ccd48e2c817ac3751e921c99520e8d73a..afe3ae278064498c222d605db4f7593e19221263 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2018-05-05  Yusuke Suzuki  <utatane.tea@gmail.com>
>+
>+        [JSC] Object.assign for final objects should be faster
>+        https://bugs.webkit.org/show_bug.cgi?id=185348
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/object-assign-fast-path.js: Added.
>+        (shouldBe):
>+        (checkProperty):
>+
> 2018-05-04  Keith Miller  <keith_miller@apple.com>
> 
>         isCacheableArrayLength should return true for undecided arrays
>diff --git a/JSTests/stress/object-assign-fast-path.js b/JSTests/stress/object-assign-fast-path.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..39562cca4aa8cdcc8939dca52c01e7ad5d2b5849
>--- /dev/null
>+++ b/JSTests/stress/object-assign-fast-path.js
>@@ -0,0 +1,100 @@
>+function shouldBe(actual, expected) {
>+    if (actual !== expected)
>+        throw new Error(`bad value: ${String(actual)}`);
>+}
>+
>+function shouldThrow(func, errorMessage) {
>+    var errorThrown = false;
>+    var error = null;
>+    try {
>+        func();
>+    } catch (e) {
>+        errorThrown = true;
>+        error = e;
>+    }
>+    if (!errorThrown)
>+        throw new Error('not thrown');
>+    if (String(error) !== errorMessage)
>+        throw new Error(`bad error: ${String(error)}`);
>+}
>+
>+function checkProperty(object, name, value, attributes = { writable: true, enumerable: true, configurable: true })
>+{
>+    var desc = Object.getOwnPropertyDescriptor(object, name);
>+    shouldBe(!!desc, true);
>+    shouldBe(desc.writable, attributes.writable);
>+    shouldBe(desc.enumerable, attributes.enumerable);
>+    shouldBe(desc.configurable, attributes.configurable);
>+    shouldBe(desc.value, value);
>+}
>+
>+{
>+    let result = Object.assign({}, RegExp);
>+    shouldBe(JSON.stringify(Object.getOwnPropertyNames(result).sort()), `["$1","$2","$3","$4","$5","$6","$7","$8","$9","input","lastMatch","lastParen","leftContext","multiline","rightContext"]`);
>+}
>+{
>+    function Hello() { }
>+    let result = Object.assign(Hello, {
>+        ok: 42
>+    });
>+
>+    shouldBe(JSON.stringify(Object.getOwnPropertyNames(result).sort()), `["arguments","caller","length","name","ok","prototype"]`);
>+    checkProperty(result, "ok", 42);
>+}
>+{
>+    let result = Object.assign({ ok: 42 }, { 0: 0, 1: 1 });
>+    shouldBe(JSON.stringify(Object.getOwnPropertyNames(result).sort()), `["0","1","ok"]`);
>+    checkProperty(result, "ok", 42);
>+    checkProperty(result, "0", 0);
>+    checkProperty(result, "1", 1);
>+}
>+{
>+    let object = { 0: 0, 1: 1 };
>+    ensureArrayStorage(object);
>+    let result = Object.assign({ ok: 42 }, object);
>+    shouldBe(JSON.stringify(Object.getOwnPropertyNames(result).sort()), `["0","1","ok"]`);
>+    checkProperty(result, "ok", 42);
>+    checkProperty(result, "0", 0);
>+    checkProperty(result, "1", 1);
>+}
>+{
>+    let called = false;
>+    let result = Object.assign({}, {
>+        get hello() {
>+            called = true;
>+            return 42;
>+        }
>+    });
>+    shouldBe(JSON.stringify(Object.getOwnPropertyNames(result).sort()), `["hello"]`);
>+    shouldBe(called, true);
>+    checkProperty(result, "hello", 42);
>+}
>+{
>+    let object = {};
>+    Object.defineProperty(object, "__proto__", {
>+        value: 42,
>+        enumerable: true,
>+        writable: true,
>+        configurable: true
>+    });
>+    checkProperty(object, "__proto__", 42);
>+    shouldBe(JSON.stringify(Object.getOwnPropertyNames(object).sort()), `["__proto__"]`);
>+    let result = Object.assign({}, object);
>+    shouldBe(JSON.stringify(Object.getOwnPropertyNames(result).sort()), `[]`);
>+    shouldBe(Object.getOwnPropertyDescriptor(result, "__proto__"), undefined);
>+    shouldBe(result.__proto__, Object.prototype);
>+}
>+{
>+    let object = {};
>+    Object.defineProperty(object, "hello", {
>+        value: 42,
>+        writable: false,
>+        enumerable: true,
>+        configurable: false
>+    });
>+    checkProperty(object, "hello", 42, { writable: false, enumerable: true, configurable: false });
>+    shouldBe(JSON.stringify(Object.getOwnPropertyNames(object).sort()), `["hello"]`);
>+    shouldThrow(() => {
>+        Object.assign(object, { hello: 50 });
>+    }, `TypeError: Attempted to assign to readonly property.`);
>+}

Actions: View | Formatted Diff | Diff

Attachments on bug 185348: 339637 | 339638 | 339639 | 339645 | 339666 | 339668 | 339671 | 339943