Update handlecache.ts (#24233) #24256
Conversation
## Description While testing the performance of SharedMatrix when loading large CSV datasets of 100k-500k rows and 10-12 columns, it was identified that cacheMiss() contributes significantly to the overall performance. This change removes the spread operator and reduces the number of copies and edits made to the handles array. Performance testing shows a 3x improvement in time taken to load large dataset, and a 50-95% reduction in memory usage.
github-actions
bot
added
area: dds
|
Warning WARNING: This PR is targeting a release branch! All changes must first be merged into Changes to release branches require approval from the Patch Triage group before merging. For more details, see our internal documentation for the patch policy and processes for |
There was a problem hiding this comment.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
packages/dds/matrix/src/handlecache.ts:75
- [nitpick] The parameter name 'handles' might be confusing since this array is used to accumulate results. Consider renaming it to 'accumulatedHandles' or 'resultHandles' for clarity.
private getHandles(start: number, end: number, handles: Handle[]): Handle[] {
packages/dds/matrix/src/handlecache.ts:72
- [nitpick] The doc comment for getHandles should be updated to clarify that the function appends results to a provided accumulator array instead of returning a new array, to avoid confusion for future maintainers.
/**
| // TODO: This can be accelerated substantially using 'walkSegments()'. The only catch | ||
| // is that | ||
|
|
||
| const handles: Handle[] = []; | ||
| const { vector } = this; | ||
|
|
||
| for (let pos = start; pos < end; pos++) { |
There was a problem hiding this comment.
I'd think we want to remove return handles; statement at the bottom, as not used / useful?
Description
This is a cherry-pick of a performance improvement commit in order to patch the latest 2.31 client release.
While testing the performance of SharedMatrix when loading large CSV datasets of 100k-500k rows and 10-12 columns, it was identified that cacheMiss() contributes significantly to the overall performance. This change removes the spread operator and reduces the number of copies and edits made to the handles array. Performance testing shows a 3x improvement in time taken to load large dataset, and a 50-95% reduction in memory usage.
Original PR: #24233