WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Patch
(24.64 KB, patch)
2021-11-23 23:39 PST
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(30.10 KB, patch)
2021-11-24 22:15 PST
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(30.30 KB, patch)
2021-11-24 22:18 PST
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(28.55 KB, patch)
2021-11-24 22:27 PST
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Patch
(34.10 KB, patch)
2021-11-25 17:37 PST
,
Yusuke Suzuki
ashvayka
: review+
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
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
<
rdar://problem/85050031
>
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
Created
attachment 445071
[details]
Patch
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
Created
attachment 445119
[details]
Patch
Yusuke Suzuki
Comment 9
2021-11-24 22:18:12 PST
Created
attachment 445120
[details]
Patch
Yusuke Suzuki
Comment 10
2021-11-24 22:27:22 PST
Created
attachment 445121
[details]
Patch
Yusuke Suzuki
Comment 11
2021-11-25 17:37:26 PST
Created
attachment 445163
[details]
Patch
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
Committed
r286251
(
244614@main
): <
https://commits.webkit.org/244614@main
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug