RESOLVED FIXED 232479
[JSC] Public Class Field initialization is slow
https://bugs.webkit.org/show_bug.cgi?id=232479
Summary [JSC] Public Class Field initialization is slow
Rob Palmer
Reported 2021-10-29 00:53:29 PDT
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
Attachments
Field Initialization Benchmark (3.62 KB, text/javascript)
2021-10-29 00:53 PDT, Rob Palmer
no flags
Patch (24.64 KB, patch)
2021-11-23 23:39 PST, Yusuke Suzuki
no flags
Patch (30.10 KB, patch)
2021-11-24 22:15 PST, Yusuke Suzuki
no flags
Patch (30.30 KB, patch)
2021-11-24 22:18 PST, Yusuke Suzuki
no flags
Patch (28.55 KB, patch)
2021-11-24 22:27 PST, Yusuke Suzuki
no flags
Patch (34.10 KB, patch)
2021-11-25 17:37 PST, Yusuke Suzuki
ashvayka: review+
Caitlin Potter (:caitp)
Comment 1 2021-11-02 07:58:24 PDT
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
Comment 2 2021-11-05 00:54:16 PDT
Yusuke Suzuki
Comment 3 2021-11-23 23:01:52 PST
*** Bug 198330 has been marked as a duplicate of this bug. ***
Yusuke Suzuki
Comment 4 2021-11-23 23:35:07 PST
Yeah, looks like public-class-field implementation lacks this optimization.
Yusuke Suzuki
Comment 5 2021-11-23 23:39:26 PST
Caitlin Potter (:caitp)
Comment 6 2021-11-24 06:50:32 PST
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 7 2021-11-24 22:14:21 PST
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
Comment 8 2021-11-24 22:15:20 PST
Yusuke Suzuki
Comment 9 2021-11-24 22:18:12 PST
Yusuke Suzuki
Comment 10 2021-11-24 22:27:22 PST
Yusuke Suzuki
Comment 11 2021-11-25 17:37:26 PST
Alexey Shvayka
Comment 12 2021-11-29 09:23:38 PST
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 13 2021-11-29 12:02:51 PST
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
Comment 14 2021-11-29 12:04:21 PST
Note You need to log in before you can comment on or make changes to this bug.