* SUMMARY Redefining a property should not change its index in Object.keys. * TEST: var o = {}; o.x = 1; o.y = 2; o.__defineGetter__("x", function() {}); Object.keys(o); * EXPECTED ["x", "y"] * ACTUAL ["y", "x"] * NOTES - Firefox outputs expected - Chrome outputs expected
My guess would be the deleteProperty called from JSObject::defineOwnNonIndexProperty, removes the "x" and then re-adds "x" to the end. We should have a replace here, that avoids the deletion.
Relevant parts of the spec are: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-validateandapplypropertydescriptor - [[DefineOwnProperty]] -> ValidateAndApplyPropertyDescriptor -> "Convert the property ... from a data property to an accessor property" https://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinary-object-internal-methods-and-internal-slots-ownpropertykeys - [[OwnPropertyKeys]] -> enumerates indexes, properties "in property creation order", and Symbols in proprty creation order Though __defineGetter__ and __defineSetter__ are not specced, this should be no different then a Object.defineProperty changing a data descriptor to an accessor (and vice versa). Now lets see if there is a JSC internal reason for the property deletion.
Doesn't appear to be any particular reason. The implementation has always had a delete since Object.defineProperty was first added for ES5 support years ago. ES5 has the update wording here.
Hah, the ChangeLog had: > Currently defineOwnProperty uses a delete followed by a put to redefine > attributes of a property, clearly this is less efficient than it could be > but we can improve this if it needs to be possible in future. I guess now is that time! But necessarily for efficiency =).
Created attachment 249247 [details] [PATCH] Work In Progress - Needs Cleanup Work in progress, not for review. Just saving my progress. I'll likely not pick this up again for a few weeks. - passes all tests as written - need to write many new tests to test this - needs some cleanup, as noted by FIXMEs - we may be able to remove DefineOwnPropertyScope and its uses throughout JSC
Created attachment 398372 [details] Patch
Comment on attachment 398372 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398372&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4325 > + push(@implContent, " JSCell::deleteProperty(this, globalObject(), propertyName);\n"); I will make sure to ASSERT delete success here and below, just making sure tests pass for now. > JSTests/test262/expectations.yaml:1012 > + default: 'Test262Error: Expected [a, name] and [name, a] to have the same contents. ' We are dealing with JSFunction's non-reified "name" property, hence the failure here and below (for "length").
Created attachment 398417 [details] Patch Set hasReadOnlyOrGetterSetterPropertiesExcludingProto(), unset PropertyAttribute::ReadOnly for accessors, adjust tests.
Created attachment 398787 [details] Patch Bring back DeletePropertyMode, always call attributeChangeTransition(), add tests and microbenchmarks.
(In reply to Alexey Shvayka from comment #9) > Bring back DeletePropertyMode, always call attributeChangeTransition(), add > tests and microbenchmarks. While this patch enables complete removal of DeletePropertyMode, I'd rather do it in a follow-up, along with implementing CanDeclareGlobalFunction for JSGlobalObject::addFunction(). Cold runs, --inner 4: r261263 patch redefine-property-accessor-dictionary 64.1904+-0.5412 ^ 39.7252+-2.3096 ^ definitely 1.6159x faster redefine-property-accessor 22.1282+-0.3532 ^ 21.0878+-0.5099 ^ definitely 1.0493x faster redefine-property-data-dictionary 60.6094+-0.4155 ^ 32.5806+-1.2206 ^ definitely 1.8603x faster redefine-property-data 18.8928+-1.0337 ^ 16.7405+-0.7271 ^ definitely 1.1286x faster <geometric> 35.7080+-0.3323 ^ 25.9930+-0.4242 ^ definitely 1.3738x faster
(In reply to Alexey Shvayka from comment #10) > (In reply to Alexey Shvayka from comment #9) > > Bring back DeletePropertyMode, always call attributeChangeTransition(), add > > tests and microbenchmarks. > > While this patch enables complete removal of DeletePropertyMode, I'd rather > do it in a follow-up, along with implementing CanDeclareGlobalFunction for > JSGlobalObject::addFunction(). > > > Cold runs, --inner 4: > > r261263 > patch > > redefine-property-accessor-dictionary 64.1904+-0.5412 ^ > 39.7252+-2.3096 ^ definitely 1.6159x faster > redefine-property-accessor 22.1282+-0.3532 ^ > 21.0878+-0.5099 ^ definitely 1.0493x faster > redefine-property-data-dictionary 60.6094+-0.4155 ^ > 32.5806+-1.2206 ^ definitely 1.8603x faster > redefine-property-data 18.8928+-1.0337 ^ > 16.7405+-0.7271 ^ definitely 1.1286x faster > > <geometric> 35.7080+-0.3323 ^ > 25.9930+-0.4242 ^ definitely 1.3738x faster Awesome!
Comment on attachment 398787 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398787&action=review Looks nice. But I think some of Structures' flags are missing. > Source/JavaScriptCore/runtime/Structure.cpp:676 > + if (attributes & (PropertyAttribute::ReadOnly | PropertyAttribute::Accessor)) > + structure->setHasReadOnlyOrGetterSetterPropertiesExcludingProto(true); Do we ensure that propertyName is not __proto__? > Source/JavaScriptCore/runtime/Structure.cpp:677 > + Is it OK not to set setHasGetterSetterProperties / setHasCustomGetterSetterProperties etc. Can you review all the flags of Structure?
Created attachment 399662 [details] Patch Use setContainsReadOnlyProperties(), improve tests, and tweak ChangeLog.
(In reply to Yusuke Suzuki from comment #12) > Is it OK not to set setHasGetterSetterProperties / > setHasCustomGetterSetterProperties etc. Can you review all the flags of > Structure? Thank you for review, Yusuke. I've vetted all the flags and seems like we are safe here: all call sites of putDirectInternal<PutModeDefineOwnProperty> perform setHasGetterSetterPropertiesWithProtoCheck(), including custom accessors. __proto__ property creation is also handled earlier in Structure::add(). Tests are updated to cover __proto__ and indexed properties.
Created attachment 402168 [details] Patch Rebase test262/expectations.yaml and simplify test added in r263070.
Comment on attachment 402168 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402168&action=review > Source/JavaScriptCore/runtime/ErrorInstance.cpp:108 > + exception->putDirect(vm, vm.propertyNames->message, jsString(vm, message), static_cast<unsigned>(PropertyAttribute::DontEnum)); should this be its own patch? > Source/JavaScriptCore/runtime/JSObject.cpp:3583 > + object->putDirect(vm, propertyName, value, descriptor.attributes() & ~PropertyAttribute::Accessor); why are you masking out accessor? Why is that not an assert? > Source/JavaScriptCore/runtime/JSObject.cpp:3654 > + object->putDirect(vm, propertyName, value, attributes & ~PropertyAttribute::Accessor); why is masking needed? > Source/JavaScriptCore/runtime/JSObjectInlines.h:317 > + if (mode == PutModeDefineOwnProperty && (attributes != currentAttributes || (attributes & PropertyAttribute::AccessorOrCustomAccessorOrValue))) so we always transition for defining a new accessor? Is this to maintain the invariant before that we did delete? I think semantically we only care about doing this for customs and custom values
Awesome!
(In reply to Saam Barati from comment #16) I appreciate you taking a look, Saam. > > Source/JavaScriptCore/runtime/ErrorInstance.cpp:108 > > + exception->putDirect(vm, vm.propertyNames->message, jsString(vm, message), static_cast<unsigned>(PropertyAttribute::DontEnum)); > > should this be its own patch? We can land ErrorInstance.cpp change before the main patch, but we can't make it a follow-up since JSTests/stress/native-error-properties.js fails w/o it. Because of the changes in JSObjectInlines.h, we need to explicitly set attributes to avoid Structure::attributeChangeTransition() branch. With this patch, putDirectWithoutTransition() will fail loudly if data property already exists with different attributes. > > Source/JavaScriptCore/runtime/JSObject.cpp:3583 > > + object->putDirect(vm, propertyName, value, descriptor.attributes() & ~PropertyAttribute::Accessor); > > why are you masking out accessor? Why is that not an assert? > > > Source/JavaScriptCore/runtime/JSObject.cpp:3654 > > + object->putDirect(vm, propertyName, value, attributes & ~PropertyAttribute::Accessor); > > why is masking needed? Masking doesn't feel right to me either; please note that I am not adding new instances, but merely preserving ones we had in putDescriptor(). Since there are similar masking outs in putIndexedDescriptor(), we can make a follow-up that moves them to PropertyDescriptor::attributesOverridingCurrent(). > > Source/JavaScriptCore/runtime/JSObjectInlines.h:317 > > + if (mode == PutModeDefineOwnProperty && (attributes != currentAttributes || (attributes & PropertyAttribute::AccessorOrCustomAccessorOrValue))) > > so we always transition for defining a new accessor? Is this to maintain the > invariant before that we did delete? JSTests/stress/intrinsic-getter-with-poly-proto-getter-change.js fails if we don't preserve this invariant. Please also note `current.equalTo(globalObject, descriptor)` check that avoids putDirect* route if accessor descriptor's [[Get]] and [[Set]] are the same. (In reply to Joseph Pecoraro from comment #17) > Awesome! Thank you for sharing WIP patch!
Created attachment 403690 [details] Patch Rebase test262 expectations.
Comment on attachment 403690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403690&action=review > Source/JavaScriptCore/runtime/ErrorInstance.cpp:108 > - exception->putDirect(vm, vm.propertyNames->message, jsString(vm, message)); > + exception->putDirect(vm, vm.propertyNames->message, jsString(vm, message), static_cast<unsigned>(PropertyAttribute::DontEnum)); Should this be separate? At the least it should be called out in the ChangeLog. > LayoutTests/js/script-tests/object-literal-duplicate-properties.js:-57 > -// FIXME: <https://webkit.org/b/142933> Redefining a property should not change its insertion index (Object.keys order) Awesome! And since we are here can we drive-by update the comment on line 12 to say "... eliminated by basic properties.". This typo of mine has annoyed me for a while!
(In reply to Joseph Pecoraro from comment #20) > Should this be separate? At the least it should be called out in the > ChangeLog. Due to a reason mentioned in comment #16, we can't make it a follow-up, so let's land it first: https://webkit.org/b/214089. > Awesome! And since we are here can we drive-by update the comment on line 12 > to say "... eliminated by basic properties.". This typo of mine has annoyed > me for a while! Thank you, I will fix it during next rebase.
Created attachment 403934 [details] Patch Rebase patch to drop ErrorInstance.cpp change and fix typos.
(In reply to Alexey Shvayka from comment #18) > (In reply to Saam Barati from comment #16) > > I appreciate you taking a look, Saam. > > > > Source/JavaScriptCore/runtime/ErrorInstance.cpp:108 > > > + exception->putDirect(vm, vm.propertyNames->message, jsString(vm, message), static_cast<unsigned>(PropertyAttribute::DontEnum)); > > > > should this be its own patch? > > We can land ErrorInstance.cpp change before the main patch, but we can't > make it a follow-up since JSTests/stress/native-error-properties.js fails > w/o it. > Because of the changes in JSObjectInlines.h, we need to explicitly set > attributes to avoid Structure::attributeChangeTransition() branch. > With this patch, putDirectWithoutTransition() will fail loudly if data > property already exists with different attributes. > > > > Source/JavaScriptCore/runtime/JSObject.cpp:3583 > > > + object->putDirect(vm, propertyName, value, descriptor.attributes() & ~PropertyAttribute::Accessor); > > > > why are you masking out accessor? Why is that not an assert? > > > > > Source/JavaScriptCore/runtime/JSObject.cpp:3654 > > > + object->putDirect(vm, propertyName, value, attributes & ~PropertyAttribute::Accessor); > > > > why is masking needed? > > Masking doesn't feel right to me either; please note that I am not adding > new instances, but merely preserving ones we had in putDescriptor(). > Since there are similar masking outs in putIndexedDescriptor(), we can make > a follow-up that moves them to > PropertyDescriptor::attributesOverridingCurrent(). > > > > Source/JavaScriptCore/runtime/JSObjectInlines.h:317 > > > + if (mode == PutModeDefineOwnProperty && (attributes != currentAttributes || (attributes & PropertyAttribute::AccessorOrCustomAccessorOrValue))) > > > > so we always transition for defining a new accessor? Is this to maintain the > > invariant before that we did delete? > > JSTests/stress/intrinsic-getter-with-poly-proto-getter-change.js fails if we > don't preserve this invariant. > Please also note `current.equalTo(globalObject, descriptor)` check that > avoids putDirect* route if accessor descriptor's [[Get]] and [[Set]] are the > same. I think we need to transition when changing custom value/custom accessor. But changing GetterSetter without transitioning should work, since all of our ICs are dynamic over this value. > > > > (In reply to Joseph Pecoraro from comment #17) > > Awesome! > > Thank you for sharing WIP patch!
Comment on attachment 403934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403934&action=review r=me with one comment/question > Source/JavaScriptCore/ChangeLog:61 > - beind redefined with PropertyAttribute::None. > + being redefined with PropertyAttribute::None. no need to fix this now IMO. We usually don't go back and fix ChangeLog typos afaik. > Source/JavaScriptCore/runtime/JSObjectInlines.h:317 > + if (mode == PutModeDefineOwnProperty && (attributes != currentAttributes || (attributes & PropertyAttribute::AccessorOrCustomAccessorOrValue))) same comment here. I think we should be ok with just CustomAccessorOrValue. Do tests failing just using CustomAccessorOrValue? If so, we should find out why.
Created attachment 404321 [details] Patch Fix flaky microbenchmarks, add FIXMEs, undo ChangeLog typo fix.
(In reply to Saam Barati from comment #24) > r=me with one comment/question Thank you for review! > same comment here. I think we should be ok with just CustomAccessorOrValue. > Do tests failing just using CustomAccessorOrValue? If so, we should find out > why. Only poly prototype __proto__ test (JSTests/stress/intrinsic-getter-with-poly-proto-getter-change.js) fails. Changing non-intrinsic GetterSetter w/o transition works and is tested by JSTests/stress/redefine-property-{get,set}.js. I've filed https://bugs.webkit.org/show_bug.cgi?id=214342 and added FIXMEs.
Committed r264574: <https://trac.webkit.org/changeset/264574> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404321 [details].
<rdar://problem/65776081>
This change causes 10% regression in Speedometer2/Ember-Debug. Can you take a look?
*** Bug 202139 has been marked as a duplicate of this bug. ***
(In reply to Yusuke Suzuki from comment #29) > This change causes 10% regression in Speedometer2/Ember-Debug. Can you take a look? Did this ever get looked at?
(In reply to Joseph Pecoraro from comment #31) > (In reply to Yusuke Suzuki from comment #29) > > This change causes 10% regression in Speedometer2/Ember-Debug. Can you take a look? > > Did this ever get looked at? Yes, this regression was fixed in r265642.