Proxy's "ownKeys" trap result should not be sorted
Created attachment 408127 [details] Patch
(In reply to Alexey Shvayka from comment #1) > Created attachment 408127 [details] > Patch r266637 patch object-get-own-property-symbols 71.3406+-2.3277 ? 74.9081+-1.3303 ? might be 1.0500x slower object-get-own-property-symbols-on-large-array 63.5366+-1.0785 61.9418+-0.9174 might be 1.0257x faster reflect-own-keys 77.5988+-0.7745 77.2685+-0.6758 Not sure why there is a slight Object.getOwnPropertySymbols() slowdown: the patch avoids second PropertyTable iteration for PropertyNameMode::Symbols. This change also corrupts Symbol.iterator order observed by Reflect.ownKeys() on loose-mode `arguments` object with a custom string property is defined: ``` (function() { "use loose"; arguments.foo = 1; return Reflect.ownKeys(arguments); })(1, 2, 3) ``` IMO it's fine to land this change since a) ^ is a nearly-impossible code and b) we already report incorrect property order for lazily-materialized properties: "lastIndex", function's "name" and "length", strict-mode `arguments` etc.
Created attachment 408139 [details] Patch Fix incorrect assert in JSTests/stress/proxy-own-keys.js.
Comment on attachment 408139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408139&action=review r=me > Source/JavaScriptCore/runtime/ObjectConstructor.cpp:369 > + ASSERT(propertyName.isSymbol() ? !propertyName.isPrivateName() : true); Let's use `ASSERT(!propertyName.isPrivateName())`.
<rdar://problem/68807597>
Created attachment 408677 [details] Patch Tweak ASSERT and reflect microbenchmark results in ChangeLog.
(In reply to Alexey Shvayka from comment #2) > Not sure why there is a slight Object.getOwnPropertySymbols() slowdown: the > patch avoids second PropertyTable iteration for PropertyNameMode::Symbols. Thank you for review! cq? due to microbenchmarks results (--outer 200): object-keys 23.4018+-0.5233 ! 24.7830+-0.3976 ! definitely 1.0590x slower object-get-own-property-symbols 63.0929+-0.5759 ! 70.7406+-0.6986 ! definitely 1.1212x slower The slowdown is caused by grown Structure::getPropertyNamesFromStructure() code size. Is this something we can fix?
(In reply to Alexey Shvayka from comment #7) > (In reply to Alexey Shvayka from comment #2) > > Not sure why there is a slight Object.getOwnPropertySymbols() slowdown: the > > patch avoids second PropertyTable iteration for PropertyNameMode::Symbols. > > Thank you for review! cq? due to microbenchmarks results (--outer 200): > > object-keys 23.4018+-0.5233 ! > 24.7830+-0.3976 ! definitely 1.0590x slower > object-get-own-property-symbols 63.0929+-0.5759 ! > 70.7406+-0.6986 ! definitely 1.1212x slower > > The slowdown is caused by grown Structure::getPropertyNamesFromStructure() > code size. > Is this something we can fix? I'm not sure we care about this small of a regression on a microbenchmark if it doesn't show up on a macrobenchmark. However, if we find out we care, we can maybe templatize the function on different enumeration modes. However, I'd say just land it as is right now.
Comment on attachment 408677 [details] Patch (In reply to Saam Barati from comment #8) > However, if we find out we care, we can maybe templatize the function on > different enumeration modes. > > However, I'd say just land it as is right now. Also, we will regain most of the slowdown by caching Object.getOwnPropertySymbols() similar to r265934.
Committed r267040: <https://trac.webkit.org/changeset/267040> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408677 [details].