[JSC] Public Class Field initialization is slow

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Rob Palmer]

Created attachment 442791 [details]
Field Initialization Benchmark

I benchmarked ES2022 class field initialization time.

  https://gist.github.com/robpalme/a9ff3e4764b764d7dc24743816d8a624#gistcomment-3944128

In Chrome and Firefox, public field initialization has been optimized to have similar performance to private fields.  Performance is also similar to assigning to `this` inside the constructor.

In latest Safari Tech Preview 134, performance of public field initialization is 12x worse than both private fields and constructor assignment.

- Classic Constructor Assignments:  38ms
- Public Fields:  462ms
- Private Fields:  33ms

[Caitlin Potter (:caitp)]

Currently, public fields are implemented in LLint as op_define_data_property and op_define_accessor_property, both of which seem to be implemented only as slow paths with no caching, and as C++ calls with no caching in each JIT.

It may be worth adding some caching and inlining for these operations.

[Radar WebKit Bug Importer]

<rdar://problem/85050031>

[Yusuke Suzuki]

*** Bug 198330 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Yeah, looks like public-class-field implementation lacks this optimization.

[Yusuke Suzuki]

Created attachment 445071 [details]
Patch

[Caitlin Potter (:caitp)]

Comment on attachment 445071 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSObject.h:1590
> +    return putDirectInternal<PutModeDefineOwnPropertyIgnoringExtensibility>(vm, propertyName, value, attributes, slot);

Under which conditions do we end up with the "IgnoringExtensibility" flag? Is this a type speculation to avoid unnecessary checks in DFG?

Under normal circumstances, defining public class fields should be concerned with extensibility, particularly for derived classes

[Yusuke Suzuki]

Comment on attachment 445071 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSObject.h:1590
>> +    return putDirectInternal<PutModeDefineOwnPropertyIgnoringExtensibility>(vm, propertyName, value, attributes, slot);
> 
> Under which conditions do we end up with the "IgnoringExtensibility" flag? Is this a type speculation to avoid unnecessary checks in DFG?
> 
> Under normal circumstances, defining public class fields should be concerned with extensibility, particularly for derived classes

For now, I would like to keep the current putDirect semantics since this patch's intention is not changing that.
Probably, I'll look into it to remove that hack for putDirect. But for now, I keep that semantics and adding the new cleaner one.

[Yusuke Suzuki]

Created attachment 445119 [details]
Patch

[Yusuke Suzuki]

Created attachment 445120 [details]
Patch

[Yusuke Suzuki]

Created attachment 445121 [details]
Patch

[Yusuke Suzuki]

Created attachment 445163 [details]
Patch

[Alexey Shvayka]

Comment on attachment 445163 [details]
Patch

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

r=me with a tiny nit on FIXME.

So nice to see generator.emitCallDefineProperty() gone! I've evaluated pretty much the same approach to replace them in ClassExprNode::emitBytecode() but that didn't result in microbenchmark progression.

Also, really cool to have JSObject::putDirectInternal() return an error message, improving developer experience with JSC.

> Source/JavaScriptCore/runtime/CommonSlowPaths.h:202
> +    if (LIKELY(propertyName != vm.propertyNames->underscoreProto && !structure->hasReadOnlyOrGetterSetterPropertiesExcludingProto() && (isJSFunction || structure->classInfo()->methodTable.defineOwnProperty == &JSObject::defineOwnProperty))) {

This handle non-reified accessors, great!

> Source/JavaScriptCore/runtime/JSObjectInlines.h:285
>      // FIXME: For a failure due to non-extensible structure, the error message is misleading

We can remove this FIXME now.

[Yusuke Suzuki]

Comment on attachment 445163 [details]
Patch

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

Thank you!

>> Source/JavaScriptCore/runtime/CommonSlowPaths.h:202
>> +    if (LIKELY(propertyName != vm.propertyNames->underscoreProto && !structure->hasReadOnlyOrGetterSetterPropertiesExcludingProto() && (isJSFunction || structure->classInfo()->methodTable.defineOwnProperty == &JSObject::defineOwnProperty))) {
> 
> This handle non-reified accessors, great!

:D

>> Source/JavaScriptCore/runtime/JSObjectInlines.h:285
>>      // FIXME: For a failure due to non-extensible structure, the error message is misleading
> 
> We can remove this FIXME now.

NICE.

[Yusuke Suzuki]

Committed r286251 (244614@main): <https://commits.webkit.org/244614@main>