RESOLVED FIXED Bug 216227
Proxy's "ownKeys" trap result should not be sorted
https://bugs.webkit.org/show_bug.cgi?id=216227
Summary Proxy's "ownKeys" trap result should not be sorted
Alexey Shvayka
Reported 2020-09-06 10:27:18 PDT
Proxy's "ownKeys" trap result should not be sorted
Attachments
Patch (16.70 KB, patch)
2020-09-06 11:40 PDT, Alexey Shvayka
no flags
Patch (17.30 KB, patch)
2020-09-06 15:23 PDT, Alexey Shvayka
no flags
Patch (17.37 KB, patch)
2020-09-13 20:50 PDT, Alexey Shvayka
no flags
Alexey Shvayka
Comment 1 2020-09-06 11:40:54 PDT
Alexey Shvayka
Comment 2 2020-09-06 11:53:56 PDT
(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.
Alexey Shvayka
Comment 3 2020-09-06 15:23:20 PDT
Created attachment 408139 [details] Patch Fix incorrect assert in JSTests/stress/proxy-own-keys.js.
Yusuke Suzuki
Comment 4 2020-09-09 14:32:21 PDT
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())`.
Radar WebKit Bug Importer
Comment 5 2020-09-13 10:28:16 PDT
Alexey Shvayka
Comment 6 2020-09-13 20:50:03 PDT
Created attachment 408677 [details] Patch Tweak ASSERT and reflect microbenchmark results in ChangeLog.
Alexey Shvayka
Comment 7 2020-09-13 20:53:36 PDT
(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?
Saam Barati
Comment 8 2020-09-14 09:52:54 PDT
(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.
Alexey Shvayka
Comment 9 2020-09-14 14:24:06 PDT
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.
EWS
Comment 10 2020-09-14 14:30:30 PDT
Committed r267040: <https://trac.webkit.org/changeset/267040> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408677 [details].
Note You need to log in before you can comment on or make changes to this bug.