Bug 212954 – [JSC] Simplify get*PropertyNames() methods and EnumerationMode

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

JSAPIValueWrapper doesn't override any of the forms of getPropertyNames.  Hence, there probably is no reason to have this flag, but we should investigate deeper before removing it.

If the flag can be remove, then we can strengthen the assertion for overridesAnyFormOfGetPropertyNames() in Structure::validateFlags().

[Mark Lam]

Ditto for JSGlobalObject.  We should investigate whether the flag is really needed.

[Mark Lam]

Closed the wrong bug.  Re-opening.

[Alexey Shvayka]

Created attachment 416300 [details]
Patch

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #3)
> Created attachment 416300 [details]
> Patch

                                       r270393                    patch

object-get-own-property-symbols    64.6840+-0.9261     ^     61.2901+-0.6377        ^ definitely 1.0554x faster
object-keys-cloned-arguments       64.2298+-0.9984     ^     52.0452+-1.3066        ^ definitely 1.2341x faster
object-keys-error-object          392.5292+-9.7568     ^     77.6232+-1.5012        ^ definitely 5.0569x faster

[Alexey Shvayka]

Created attachment 416418 [details]
Patch

Fix early return condition in getNonReifiedStaticPropertyNames() and export getOwnSpecialPropertyNames().

[Yusuke Suzuki]

Comment on attachment 416418 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=416418&action=review

r=me

> Source/JavaScriptCore/runtime/JSObject.cpp:2384
> +constexpr unsigned maximumPrototypeChainDepth = 40000;

Let's put `static ` before this. And maybe, defining it in JSObject's class static field (JSObject::maximumPrototypeChainDepth) is better.

> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.cpp:110
> +        // Inlined JSObject::getOwnNonIndexPropertyNames()
> +        base->methodTable(vm)->getOwnSpecialPropertyNames(base, globalObject, propertyNames, DontEnumPropertiesMode::Exclude);
> +        RETURN_IF_EXCEPTION(scope, void());
> +
> +        base->getNonReifiedStaticPropertyNames(vm, propertyNames, DontEnumPropertiesMode::Exclude);
> +        unsigned nonStructurePropertyCount = propertyNames.size();
> +        structure->getPropertyNamesFromStructure(vm, propertyNames, DontEnumPropertiesMode::Exclude);
> +        scope.assertNoException();
> +
> +        // |propertyNames| contains properties exclusively from the structure.
> +        if (!nonStructurePropertyCount)
> +            structurePropertyCount = propertyNames.size();

This means that, if non-reified property exists, we cannot use fast path (since nonStructurePropertyCount is non-zero, and structurePropertyCount is always zero).
Is it the same in the old code? If the old code can use the fast path even if non-reified static property exists, can you change the new code to the same state?

[Alexey Shvayka]

Created attachment 416886 [details]
Patch

Use JSObject::maximumPrototypeChainDepth and add a test case for non-reified static properties.

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #6)
> This means that, if non-reified property exists, we cannot use fast path
> (since nonStructurePropertyCount is non-zero, and structurePropertyCount is
> always zero).
> Is it the same in the old code? If the old code can use the fast path even
> if non-reified static property exists, can you change the new code to the
> same state?

Yes, right, this is very intentional improvement over old code: to match the order required by the spec, structure properties should come after non-reified static properties, which isn't possible on the fast path because structure loop comes before generic loop.

This change is performance-neutral because slow path is taken only if there is an [[Enumerable]] non-reified or special property, which is extremely rare.
In JSC, only `WebAssembly` and `WebAssembly.Module` objects have [[Enumerable]] non-reified properties.
However, even they, as well as WebIDL interfaces like `Element.prototype`, never passed the canAccessPropertiesQuicklyForEnumeration() check because of accessors.

I've added a test for for/in with `WebAssembly`, which used to fail before https://bugs.webkit.org/show_bug.cgi?id=214908.

[Alexey Shvayka]

Created attachment 416888 [details]
Patch

Guard stress test with `WebAssembly` check.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 416888 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Alexey Shvayka]

Committed r271269: <https://trac.webkit.org/changeset/271269>

[Radar WebKit Bug Importer]

<rdar://problem/72908707>