Bug 213133 – Expand JSObject::defineOwnIndexedProperty() fast path for existing properties

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

[Alexey Shvayka]

Expand JSObject::defineOwnIndexedProperty() fast path for existing properties

[Alexey Shvayka]

Created attachment 401736 [details]
Patch

[Alexey Shvayka]

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

Warmed-up runs, --outer 128:

                                       r262942                    patch                                       

array-redefine-index-reverse       57.2178+-0.6638     ^      3.3607+-0.1611        ^ definitely 17.0256x faster
array-redefine-index               86.8468+-1.0661     ^     64.3230+-0.7674        ^ definitely 1.3502x faster

<geometric>                        70.4131+-0.5940     ^     14.6275+-0.2617        ^ definitely 4.8137x faster

[Alexey Shvayka]

Comment on attachment 401736 [details]
Patch

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

> JSTests/stress/define-own-indexed-property-fast-path.js:11
> +  for (const key of Object.keys(descriptor).sort())

Sorting can be removed when https://bugs.webkit.org/show_bug.cgi?id=142933 is fixed.

[Yusuke Suzuki]

Comment on attachment 401736 [details]
Patch

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

r=me with comments about TypedArrays.

> Source/JavaScriptCore/runtime/JSObject.cpp:2626
> +        static const PropertyDescriptor emptyAttributesDescriptor(jsUndefined(), static_cast<unsigned>(PropertyAttribute::None));

We should not use static here. Creating PropertyDescriptor is not costly.
And let's put `ASSERT(emptyAttributesDescriptor.attributes() == PropertyAttribute::None)`.

> Source/JavaScriptCore/runtime/JSObject.cpp:2635
> +#if ASSERT_ENABLED
> +        if (canGetIndexQuickly(index)) {
> +            PropertyDescriptor currentDescriptor;
> +            ASSERT(getOwnPropertyDescriptor(globalObject, Identifier::from(vm, index), currentDescriptor));
> +            scope.assertNoException();
> +            ASSERT(currentDescriptor.attributes() == emptyAttributesDescriptor.attributes());
> +        }
> +#endif

I think this is not correct for typed-arrays. Can you add tests and fix this?
While canDoFastPutDirectIndex can prevent using putDirectIndex for typed-arrays, but this is executed for typed-arrays too.

[Alexey Shvayka]

Created attachment 401918 [details]
Patch

Drop `static`, add PropertyAttribute::None ASSERT, fix canGetIndexQuickly() ASSERT.

[Alexey Shvayka]

(In reply to Yusuke Suzuki from comment #4)

Thank you for review!

> I think this is not correct for typed-arrays. Can you add tests and fix this?
> While canDoFastPutDirectIndex can prevent using putDirectIndex for
> typed-arrays, but this is executed for typed-arrays too.

JSGenericTypedArrayView<Adaptor>::defineOwnProperty() appears to be called instead of JSObject::defineOwnIndexedProperty(), that is why no tests are failing.
I've added canDoFastPutDirectIndex() check to the assert nonetheless.

[[DefineOwnProperty]] on typed arrays is covered by JSTests/stress/typedarray-configure-index.js, as well as by test262 suite:
https://test262.report/browse/built-ins/TypedArrayConstructors/internals/DefineOwnProperty.

[EWS]

Committed r263070: <https://trac.webkit.org/changeset/263070>

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

[Radar WebKit Bug Importer]

<rdar://problem/64387661>