WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(17.30 KB, patch)
2020-09-06 15:23 PDT
,
Alexey Shvayka
no flags
Details
Formatted Diff
Diff
Patch
(17.37 KB, patch)
2020-09-13 20:50 PDT
,
Alexey Shvayka
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Alexey Shvayka
Comment 1
2020-09-06 11:40:54 PDT
Created
attachment 408127
[details]
Patch
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
<
rdar://problem/68807597
>
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.
Top of Page
Format For Printing
XML
Clone This Bug