perf(aggregate): cache sorted values for percentile/median to avoid repeated sorts #105
Conversation
|
This PR will trigger a patch release when merged. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR adds performance optimization to the Aggregate class by caching sorted values for percentile and median calculations. Instead of sorting the values array on every percentile call, the array is sorted once in-place and a _sorted flag tracks whether sorting has occurred.
Key changes:
- Added
_sortedflag to track whether values array has been sorted - Modified
percentile()method to sort values array only once on first call - Subsequent percentile/median calls reuse the cached sorted array
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!this._sorted) { | ||
| // Sort once in place; all other metrics are order‑insensitive | ||
| this.values.sort((left, right) => left - right); | ||
| this._sorted = true; | ||
| } |
There was a problem hiding this comment.
The caching logic is fragile and could lead to incorrect results if values are added after percentile() is called. The _sorted flag is never reset when new values are pushed to this.values (e.g., via aggregateFn at line 92).
Consider either:
- Making the
valuesarray immutable after first access (document this invariant) - Invalidating
_sortedwhen values are added (requires adding a setter/method for adding values) - Removing the cache and accepting the performance cost of sorting on each call
While the current usage pattern (build once via reduce, then query) appears safe, this implementation is error-prone for future modifications.
| if (!this._sorted) { | ||
| // Sort once in place; all other metrics are order‑insensitive | ||
| this.values.sort((left, right) => left - right); | ||
| this._sorted = true; | ||
| } | ||
| const index = Math.floor((p / 100) * this.values.length); | ||
| return this.values[index]; |
There was a problem hiding this comment.
The new caching behavior for percentile calculations lacks explicit test coverage. While existing tests call percentile() multiple times, there's no test that verifies:
- The cache is properly set after the first call
- Subsequent calls use the cached sorted array (performance test)
- The invariant that values should not be added after percentile is called
Consider adding a test that verifies multiple percentile calls return consistent results and that the array is only sorted once.
Co-authored-by: Copilot <[email protected]>
No description provided.