-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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(collection): optimisations #10552
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10552 +/- ##
==========================================
- Coverage 38.00% 37.96% -0.05%
==========================================
Files 239 239
Lines 15488 15439 -49
Branches 1367 1364 -3
==========================================
- Hits 5886 5861 -25
+ Misses 9587 9563 -24
Partials 15 15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
359f3eb
to
f4e3299
Compare
Should we add benchmarks for this? Would be easier to see the performance enhancements. |
- do not perform iterable-to-array until required - test ! before <
- skip iterable-to-array for returning single value - short-circuit if amount or collection size is zero
addfa97
to
b93437c
Compare
if (index >= 0) { | ||
if (index >= this.size) return undefined; | ||
} else { | ||
if (index < this.size * -1) return undefined; | ||
index += this.size; | ||
} |
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.
Better readability
if (index >= 0) { | |
if (index >= this.size) return undefined; | |
} else { | |
if (index < this.size * -1) return undefined; | |
index += this.size; | |
} | |
if (index < 0) { | |
index += this.size; | |
if (index < 0) return undefined; | |
} else if (index >= this.size) { | |
return undefined; | |
} |
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.
I think making one addition is faster then multiplication + addition
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.
Yeah, it's 14 8% faster for negative indexes
if (index >= 0) { | ||
if (index >= this.size) return undefined; | ||
} else { | ||
if (index < this.size * -1) return undefined; | ||
index += this.size; | ||
} |
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.
Ditto
if (index >= 0) { | |
if (index >= this.size) return undefined; | |
} else { | |
if (index < this.size * -1) return undefined; | |
index += this.size; | |
} | |
if (index < 0) { | |
index += this.size; | |
if (index < 0) return undefined; | |
} else if (index >= this.size) { | |
return undefined; | |
} |
.first()
,.firstKey()
The callback variant of
Array.from()
is slow in V8. Changing the method of array generation from:to
produces a ~90-95% reduction in execution time, when tested with all current LTS versions of Node.
Note that this is deliberately
new Array(n)
and notArray.from({ length: n })
:n
, and is an O(1) operation.n
, then for alli
in0 <= i < n
, it reads the propertyobj[i]
from the object parameter (which in this case will always be undefined), and setsarray[i]
to that value. This is significantly slower for large arrays, and obviously not what we want here.Additionally, switches to just using iterable-to-array in the case where
amount >= this.size
, which is a fast operation for builtin iterables. This further halves the execution time when compared with the above method..last()
,.lastKey()
Only carry out the memory-expensive
[...this.<iterable>()]
if needed.Previously, passing
amount <= 0
would still copy the entire collection into a temporary array, even though it's never referenced. This significantly speeds up execution in those cases.Uses the new
.at()
or.keyAt()
(as below) to fetch the last element in the collection in the case whereamount === undefined
. This is around 10-20% faster than the previous method of copying into a temporary array and selecting the last element..at()
,.keyAt()
Manually iterate to the target index, instead of generating a full array copy of the collection to call
Array.prototype.at()
.The performance of the previous implementation was essentially linear on the size of the collection, and independent of the index being fetched, as the whole collection was copied into a temporary array regardless.
The performance of the new implementation is linear on the index being fetched, and results in a performance improvement of >90% when the target index is close to the start of the collection. In the worst case when the target index is close ot the end of the collection, it still performs well (around 10-20% faster in Node v22, and around 50% faster in Node v18). It also avoids the memory cost of copying the collection into a temporary array.
This contains a small off-by-one fix which represents a technical breaking change. Previously, the implementations of
.at()
and.keyAt()
were not compliant with the standard forArray.prototype.at()
specifically when passing a negative non-integer index: the standard dictates that these are truncated (ie. rounded towards zero), whereas the previous implementation usedMath.floor()
(ie. rounded these away from zero)..random()
,.randomKey()
Changes the method of array generation to pushing to a new array, instead of passing a callback to
Array.from()
.Same rationale as
.first()
above. This change produces a ~10-40% reduction in execution time when tested with all current LTS versions of Node, depending on the magnitude of the collection size and the number of elements requested.Additionally, skips copying the collection into a temporary array if
amount === 0
, and uses the new.at()
or.keyAt()
(as above) to fetch a single element in the case whereamount === undefined
..map()
Changes the method of array generation to pushing to a new array, instead of passing a callback to
Array.from()
.Same rationale as
.first()
above. This change produces a ~80% reduction in execution time when tested with all current LTS versions of Node..merge()
Adjusts the logic for checking
hasInSelf
andhasInOther
. Previously, each of these boolean conditions was evaluated twice; this makes a small change to the control flow to deduplicate the checks..toSorted()
Removes the redundant closure wrapping
compareFunction
and just passes it directly, eliminating a needless call from the stack.Status and versioning classification: