Bug 216227

Summary: Proxy's "ownKeys" trap result should not be sorted
Product: WebKit Reporter: Alexey Shvayka <shvaikalesh>
Component: JavaScriptCoreAssignee: Alexey Shvayka <shvaikalesh>
Status: RESOLVED FIXED    
Severity: Trivial CC: ews-watchlist, keith_miller, mark.lam, msaboff, ross.kirsling, sbarati, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar, WebExposed
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Alexey Shvayka 2020-09-06 10:27:18 PDT
Proxy's "ownKeys" trap result should not be sorted
Comment 1 Alexey Shvayka 2020-09-06 11:40:54 PDT
Created attachment 408127 [details]
Patch
Comment 2 Alexey Shvayka 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.
Comment 3 Alexey Shvayka 2020-09-06 15:23:20 PDT
Created attachment 408139 [details]
Patch

Fix incorrect assert in JSTests/stress/proxy-own-keys.js.
Comment 4 Yusuke Suzuki 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())`.
Comment 5 Radar WebKit Bug Importer 2020-09-13 10:28:16 PDT
<rdar://problem/68807597>
Comment 6 Alexey Shvayka 2020-09-13 20:50:03 PDT
Created attachment 408677 [details]
Patch

Tweak ASSERT and reflect microbenchmark results in ChangeLog.
Comment 7 Alexey Shvayka 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?
Comment 8 Saam Barati 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.
Comment 9 Alexey Shvayka 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.
Comment 10 EWS 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].