Bug 193618 – Implement @copyDataProperties in C++ to optimize object rest / spread

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Let's move copyDataPropertiesNoExclusions, copyDataProperties to C++, since it is not beneficial to implement it in JS. Drop defineEnumerableWritableConfigurableDataProperty.
Then, remove @Reflect reference. Then, make Reflect lazy-initialized in JSGlobalObject.

[Alexey Shvayka]

Created attachment 417220 [details]
Patch

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #1)
> Created attachment 417220 [details]
> Patch

                                    r271191                   patch

object-spread                  178.7996+-2.7727     ^     34.7401+-0.4879        ^ definitely 5.1468x faster
object-rest-destructuring      124.6139+-2.3242     ^     39.2901+-0.4814        ^ definitely 3.1716x faster

<geometric>                    149.1572+-1.7371     ^     36.9301+-0.3394        ^ definitely 4.0389x faster

[Saam Barati]

Comment on attachment 417220 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417220&action=review

> Source/JavaScriptCore/ChangeLog:24
> +           c) unlike Object.assign, properties are copied directly in Structure::forEachProperty(),
> +              because it's guaranteed that `target` and `source` are different objects. I suspect
> +              that crashes before r234022 could be caused by code like `Object.assign(X, X)`.

what do the objects being different have to do with their structures being different? I don't follow the reasoning here. They can share the same structure, right?

Also, if this is an invariant we're relying on, let's add an assert for it.

> Source/JavaScriptCore/ChangeLog:25
> +           (+170%)

?

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:917
> +            PropertySlot slot(source, PropertySlot::InternalMethodType::GetOwnProperty);

given we do get below (in tainted case), this shouldn't be HasOwnProperty?

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:929
> +                value = source->get(globalObject, propertyName);

the spec calls for get twice?

[Yusuke Suzuki]

Comment on attachment 417220 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417220&action=review

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:907
> +        source->structure(vm)->forEachProperty(vm, [&] (const PropertyMapEntry& entry) -> bool {
> +            PropertyName propertyName(entry.key);
> +            if (propertyName.isPrivateName())
> +                return true;
> +            if (isPropertyNameExcluded(propertyName))
> +                return true;
> +            if (entry.attributes & PropertyAttribute::DontEnum)
> +                return true;
> +
> +            target->putDirect(vm, propertyName, source->getDirect(entry.offset));
> +            return true;
> +        });

For now, let's first store values into buffer and iterate them later as is done in the Object.assign.
See FIXME in Object.assign and https://bugs.webkit.org/show_bug.cgi?id=187837

[Alexey Shvayka]

(In reply to Saam Barati from comment #3)
> what do the objects being different have to do with their structures being
> different? I don't follow the reasoning here. They can share the same
> structure, right?

They can, right. Aligned with Object.assign to copy via buffer (for now).

> > Source/JavaScriptCore/ChangeLog:25
> > +           (+170%)
> 
> ?

That was microbenchmark progression for each sub-change (I was curious). Removed it as outdated (and useless).

> > Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:917
> > +            PropertySlot slot(source, PropertySlot::InternalMethodType::GetOwnProperty);
> 
> given we do get below (in tainted case), this shouldn't be HasOwnProperty?

We don't have HasOwnProperty, only HasProperty for `in`.

> > Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:929
> > +                value = source->get(globalObject, propertyName);
> 
> the spec calls for get twice?

Yes, it does (step 6.c.ii.1 of https://tc39.es/ecma262/#sec-copydataproperties), and it follows this pattern in a few other ops/built-ins, even WebIDL.

[Alexey Shvayka]

Created attachment 417227 [details]
Patch

Put properties via buffer and tweak ASSERTs.

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #6)
> Created attachment 417227 [details]
> Patch
> 
> Put properties via buffer and tweak ASSERTs.

                                    r271191                     patch

object-spread                  181.2128+-1.8450     ^     54.3278+-0.8577        ^ definitely 3.3355x faster
object-rest-destructuring      129.6295+-1.8733     ^     44.5497+-0.7565        ^ definitely 2.9098x faster

<geometric>                    153.1926+-1.0826     ^     49.1639+-0.5471        ^ definitely 3.1160x faster

===

~22% slower.

[Alexey Shvayka]

Created attachment 417229 [details]
Patch

We don't need to clear vectors.

[Yusuke Suzuki]

Comment on attachment 417229 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417229&action=review

Looking, early comments. I'll look more.

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:868
> +    JSObject* target = asObject(callFrame->uncheckedArgument(0));
> +    ASSERT(jsDynamicCast<JSFinalObject*>(vm, target));

Let's do

JSFinalObject* target = jsCast<JSFinalObject*>(allFrame->uncheckedArgument(0));

jsCast also has assert.

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:873
> +        return { };

Let's return `JSValue::encode(jsUndefined())` for non-exception path for robustness.

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:954
> +    return { };

Let's return `JSValue::encode(jsUndefined())` for non-exception path for robustness.

[Yusuke Suzuki]

Comment on attachment 417229 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=417229&action=review

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:889
> +    auto isPropertyNameExcluded = [&] (PropertyName propertyName) {
> +        ASSERT(!propertyName.isPrivateName());
> +        if (!excludedSet)
> +            return false;
> +
> +        JSValue propertyNameValue = identifierToJSValue(vm, Identifier::fromUid(vm, propertyName.uid()));
> +        return excludedSet->has(globalObject, propertyNameValue);
> +    };

Let's take `JSGlobalObject*` as a parameter in this function to make it explicit that this can throw an error.

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:912
> +            if (isPropertyNameExcluded(propertyName))
> +                return true;

We need to do an error check since this can throw an error. And if an error is thrown, we should stop iteration.

> Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:933
> +            if (isPropertyNameExcluded(propertyName))
> +                continue;

Ditto.

[Yusuke Suzuki]

Comment on attachment 417229 [details]
Patch

r=me with some comments.

[Alexey Shvayka]

Committed r271343: <https://trac.webkit.org/changeset/271343>

[Radar WebKit Bug Importer]

<rdar://problem/72954769>

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #12)
> Committed r271343: <https://trac.webkit.org/changeset/271343>

Landed accounting the comments.
test262 coverage: https://github.com/tc39/test262/pull/2930.

[Radar WebKit Bug Importer]

<rdar://problem/72954785>