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.
Created attachment 417220 [details] Patch
(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
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?
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
(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.
Created attachment 417227 [details] Patch Put properties via buffer and tweak ASSERTs.
(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.
Created attachment 417229 [details] Patch We don't need to clear vectors.
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.
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.
Comment on attachment 417229 [details] Patch r=me with some comments.
Committed r271343: <https://trac.webkit.org/changeset/271343>
<rdar://problem/72954769>
(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.
<rdar://problem/72954785>