Consider rewriting Function.prototype.bind
Categories
(Core :: JavaScript Engine, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox112 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 6 open bugs)
Details
(Whiteboard: [sp3])
Attachments
(12 files)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
Assignee | ||
Comment 1•7 years ago
|
||
Comment 2•7 years ago
|
||
Comment 3•7 years ago
|
||
Comment 4•7 years ago
|
||
Comment 5•7 years ago
|
||
Updated•7 years ago
|
Comment 6•7 years ago
|
||
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Comment 8•7 years ago
|
||
Updated•3 years ago
|
Assignee | ||
Comment 9•2 years ago
|
||
This is just removing the JIT inlining code, to simplify the next patch.
Updated•2 years ago
|
Assignee | ||
Comment 10•2 years ago
|
||
This patch rewrites Function.prototype.bind
. Instead of using a self-hosted
implementation that uses JS lambda functions, use a separate BoundFunctionObject
type and JSClass
with call/construct hooks.
Using a type other than JSFunction
matches the spec (and other implementations)
better. It also better matches the WrappedFunctionObject
implementation we use for
shadow realms. It also lets us remove some complexity from JSFunction
.
One of the main reasons for doing this is that it uses less memory: the self-hosted
implementation allocated a JSFunction
, a CallObject
, and an ArrayObject
(for bound
arguments). The new implementation allocates only a BoundFunctionObject
when we're
binding <= 3 arguments (the common case).
This patch stack improves Speedmeter's React-TodoMVC by a few percent. This test
allocates a lot of bound functions but has relatively few calls, so this is likely
due to reduced GC pressure.
The new implementation has a few minor differences in various corner cases with toSource
and the debugger. Another patch in this stack changes a few tests to account for this.
Depends on D170565
Assignee | ||
Comment 11•2 years ago
|
||
Depends on D170566
Assignee | ||
Comment 12•2 years ago
|
||
Now that bound functions use a JSClass
that's not JSFunction
, we need to give
them a JSProtoKey
to make (xray) wrapper calls work. This fixes a lot of xpcshell
and browser test failures.
The old implementation used JSProto_Function
. This patch adds JSProto_BoundFunction
and (as much as possible) tries to use the same code paths for it.
Depends on D170567
Assignee | ||
Comment 13•2 years ago
|
||
-
Debugger-onNativeCall-07.js
: the oldDebugger.Object.name
implementation was incorrect for bound functions. We now correctly get "bound call" instead of just "call" without the prefix. -
debug/Object-boundTargetFunction-02.js
,debug/Object-script-lazy.js
:isBoundFunction
no longer returns a boolean only ifisFunction
, so we now getfalse
instead ofundefined
. -
heap-analysis/byteSize-of-object.js
: bound functions are now a bit bigger, but we don't have a separateCallObject
/ArrayObject
anymore. -
saved-stacks/capture-first-frame-with-principals.js
: just use a different self-hosted function (array.map
). -
non262/extensions/uneval/function-bind.js
: for non-standard and deprecatedtoSource
, we now always return the same string. This should be okay because we no longer expose this to content.
Depends on D170568
Assignee | ||
Comment 14•2 years ago
|
||
Similar to the old implementation, add some fast paths for the common cases.
Depends on D170569
Assignee | ||
Comment 15•2 years ago
|
||
We almost always have Function.prototype
as proto. This fast path optimizes
that case.
Depends on D170570
Assignee | ||
Comment 16•2 years ago
|
||
Depends on D170571
Assignee | ||
Comment 17•2 years ago
|
||
This cache has a high hit rate so it's worth it to avoid some slow(er) StringBuffer
and atomization code.
Depends on D170572
Assignee | ||
Comment 18•2 years ago
|
||
In Ion we can now allocate the object in JIT code so the VM function only
has to initialize it. This is faster than allocating the object in C++.
Depends on D170573
Assignee | ||
Comment 19•2 years ago
|
||
For now we only handle standard non-constructor calls.
Depends on D170574
Assignee | ||
Comment 20•2 years ago
|
||
Depends on D170575
Assignee | ||
Comment 21•2 years ago
•
|
||
List of follow-up optimization ideas I'll file bugs for and look into after this lands:
- In
BoundFunctionObject::call
, reuse incomingargs
if we're only bindingthis
. Measure but probably common. - If a
bind()
site is always binding a function with the same shape/name etc, we can probably avoid a VM call. - In
tryAttachBoundFunction
, try to do a shape guard instead of a class guard with the optimistic class guard optimization. - For megamorphic bound calls, consider using a loop that handles any
numBoundArgs
. Currently harder to implement in Ion. - Bound function state is immutable. We could communicate this to
MLoadFixedSlot
so the JIT can optimize this better. - Support constructor calls to bound functions in
tryAttachCallHook
and for call-bound-scripted. - Look into bound-scripted
FunApply
, this shows up in Speedometer/React. - Investigate trial-inlining for standard calls.
- Consider flattening chains of bound functions to enable easier call optimizations. React-TodoMVC has a lot of this.
- See if scalar replacement of
bind
would help.
Comment 22•2 years ago
|
||
Comment 23•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2977375e714e
https://hg.mozilla.org/mozilla-central/rev/634f0d65b4ad
https://hg.mozilla.org/mozilla-central/rev/c5e8334fc103
https://hg.mozilla.org/mozilla-central/rev/6d2e26b555f2
https://hg.mozilla.org/mozilla-central/rev/0b80b352475a
https://hg.mozilla.org/mozilla-central/rev/f09bfbdf49f2
https://hg.mozilla.org/mozilla-central/rev/f802273d0d57
https://hg.mozilla.org/mozilla-central/rev/0b4fddb14c07
https://hg.mozilla.org/mozilla-central/rev/6ccef534e6ac
https://hg.mozilla.org/mozilla-central/rev/70e235a3de72
https://hg.mozilla.org/mozilla-central/rev/985db4d90a4e
https://hg.mozilla.org/mozilla-central/rev/443570c2740f
Assignee | ||
Comment 24•2 years ago
|
||
For posterity, this improved Speedometer's React-TodoMVC/CompletingAllItems by ~5-6% (?) and also improved some other React and React-Redux subtests a bit. Matrix-React got a little faster as well (more clear if you look at the 50/100 iterations subtests on AWFY).
Comment 25•2 years ago
•
|
||
(In reply to Jan de Mooij [:jandem] from comment #21)
List of follow-up optimization ideas I'll file bugs for and look into after this lands:
- If a
bind()
site is always binding a function with the same shape/name etc, we can probably avoid a VM call.
Jeff and Markus have profile data that indicates that this could be a valuable optimization. We're spending a solid chunk of ReactTodoMVC in the shell in FunctionBindImpl; if we can make that go away by precomputing and storing the name/etc and initializing the bound function in jit code, Jeff estimates that it would cut the difference between us and V8 by 40%.
(More discussion in the sp3 channel.)
Comment 26•2 years ago
•
|
||
Fixed link: profile data
Comment 27•2 years ago
|
||
(In reply to Iain Ireland [:iain] from comment #25)
Jeff and Markus have profile data that indicates that this could be a valuable optimization. We're spending a solid chunk of ReactTodoMVC in the shell in FunctionBindImpl; if we can make that go away by precomputing and storing the name/etc and initializing the bound function in jit code, Jeff estimates that it would cut the difference between us and V8 by 40%.
Here's where V8 does something similar by lowering JSCreateBoundFunction:
https://source.chromium.org/chromium/chromium/src/+/main:v8/src/compiler/js-create-lowering.cc;l=880;drc=6bacc8c983672cddc6d9e043472925cd60c27464
Updated•2 years ago
|
Updated•2 years ago
|
Comment 28•2 years ago
|
||
Jan, while we did not observed significant speedup in DevTools React frontend, we notified significant memory improvement.
reload-inspector:content-process memory
is especially interesting as it tanslates a significant reduction in memory usage of the DevTools Inspector while reloading the test page. It surely has a notable impact on typical developer workflow reloading their page with the inspector opened!
All other "object-with-stacks" tests are reporting a reduction is objects being potentially leaked in many tests.
I imagine that you reduced the number of internal JS objects related to bound functions? Or fixed an internal leak?
== Change summary for alert #37361 (as of Mon, 27 Feb 2023 18:54:03 GMT) ==
Regressions:
Ratio | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|
24% | reload-netmonitor:content-process objects-with-no-stacks | linux1804-64-tsan-qr | 35.71 -> 44.42 | |
10% | damp console.log-in-loop-content-process-longstring | macosx1015-64-shippable-qr | e10s fission stylo webrender-sw | 46.80 -> 51.25 |
7% | damp complicated.inspector.close.DAMP | windows10-64-shippable-qr | e10s fission stylo webrender-sw | 16.33 -> 17.42 |
6% | damp complicated.inspector.close.DAMP | windows10-64-shippable-qr | e10s fission stylo webrender | 16.30 -> 17.24 |
2% | damp console.log-in-loop-content-process-array | windows10-64-shippable-qr | e10s fission stylo webrender | 137.19 -> 139.99 |
Improvements:
Ratio | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|
100% | reload-debugger:content-process objects-with-no-stacks | linux1804-64-tsan-qr | 4.00 -> 0.00 | |
69% | reload-inspector:parent-process objects-with-stacks | linux1804-64-qr | 94.93 -> 29.00 | |
68% | toolbox:parent-process objects-with-stacks | linux1804-64-shippable-qr | 8.92 -> 2.83 | |
67% | reload-inspector:parent-process objects-with-stacks | linux1804-64-tsan-qr | 89.00 -> 29.00 | |
67% | target:parent-process objects-with-stacks | linux1804-64-qr | 3.00 -> 1.00 | |
... | ... | ... | ... | ... |
2% | reload-no-devtools:parent-process objects-with-no-stacks | linux1804-64-qr | 3,316.64 -> 3,234.67 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=37361
Assignee | ||
Comment 29•2 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #28)
All other "object-with-stacks" tests are reporting a reduction is objects being potentially leaked in many tests.
I imagine that you reduced the number of internal JS objects related to bound functions? Or fixed an internal leak?
Probably the former: we used to allocate multiple (3 or so) JS objects for a bound function and now we allocate just one. The numbers seem to reflect that.
Thanks for posting this, very interesting :)
Description
•