Bug 216227 – Proxy's "ownKeys" trap result should not be sorted

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Alexey Shvayka]

Proxy's "ownKeys" trap result should not be sorted

[Alexey Shvayka]

Created attachment 408127 [details]
Patch

[Alexey Shvayka]

(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]

Created attachment 408139 [details]
Patch

Fix incorrect assert in JSTests/stress/proxy-own-keys.js.

[Yusuke Suzuki]

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]

<rdar://problem/68807597>

[Alexey Shvayka]

Created attachment 408677 [details]
Patch

Tweak ASSERT and reflect microbenchmark results in ChangeLog.

[Alexey Shvayka]

(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]

(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 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]

Committed r267040: <https://trac.webkit.org/changeset/267040>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408677 [details].