-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: Remove costly reduce operation for generating Engine context #12374
perf: Remove costly reduce operation for generating Engine context #12374
Conversation
…s an object Adds `Object.values` operation for deriving ComposableController input array
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
2c34547
to
8774483
Compare
|
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12374 +/- ##
==========================================
+ Coverage 56.41% 56.46% +0.04%
==========================================
Files 1797 1808 +11
Lines 40586 40724 +138
Branches 5097 5131 +34
==========================================
+ Hits 22896 22993 +97
- Misses 16134 16159 +25
- Partials 1556 1572 +16 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOVE IT! LGTM! So cool that we now only have base controllers v2 controllers!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOVE IT! LGTM! So cool that we now only have base controllers v2 controllers!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Overview
Optimizes an expensive operation from the Engine initialization process.
A custom span for measuring this process was added in #11579.
Motivation
Currently, the Engine's context object is created using a
Array.prototype.reduce()
operation on a largecontrollers
array. This reduce call is also written to create a temporary object on every iteration.Benchmarks show that this particular implementation of reduce runs significantly slower than other means of creating an object, and compared to reduce calls that are written to re-use the output object the difference is in four orders of magnitude (1555.6 Ops/sec vs. 0.2 Ops/sec).
A simple fix would be to refactor the reduce operation so it doesn't rely on temporary objects and spreading:
However, we can also avoid the cost of running reduce altogether simply by creating a controller name-keyed object to assign to the Engine's context field. This is also the approach taken in the Extension
MetamaskController
.Description
controllers
array in the Engine class with a corresponding object that is directly assigned tocontext
.Object.values
for deriving ComposableController input withObject.keys
andreduce
(re-uses object).Related issues
Manual testing steps
Screenshots/Recordings
Pre-merge author checklist
Pre-merge reviewer checklist