Optimize splitAmntFast by eliminating redundant array copies in group operations#59
Conversation
- Eliminate unnecessary array copies in applyGroupCumSumFast and applyGroupDiffFast - Add groupCumSumInPlace and groupDiffInPlace for 34-53% performance gains - Optimize group lookups by caching values to avoid redundant array access - Add isolated benchmark for group operations performance testing Performance improvements (10k elements, 100 groups): - groupCumSum: 34% faster with in-place variant (11.6μs → 7.6μs) - groupDiff: 53% faster with in-place variant (10.3μs → 4.9μs) Co-authored-by: Quafadas <24899792+Quafadas@users.noreply.github.com>
Code review correctly identified that caching group values in the while condition doesn't reduce array accesses since the array is still accessed on every iteration. Co-authored-by: Quafadas <24899792+Quafadas@users.noreply.github.com>
|
@copilot what was the impact of the changes on splitamntfast? The in place grp methods don’t seem to be used? Please also validate that the test still passses? |
Impact on splitAmntFast: The optimization eliminated all array copy operations in the hot path. Previously, About the in-place methods: You're correct - the standalone Tests: All tests pass ✓ (commit 052b0fd). The test "Tower splitAmntFast vs splitAmnt - performance equivalent" verifies both implementations produce identical results. |
|
@copilot Can you re run the splitAmnt benchmark and summarise the results? |
splitAmnt Benchmark ResultsRan complete benchmark comparing 10,000 losses:
1,000,000 losses:
The optimization eliminates 4 array operations per layer (2 allocations + 2 System.arraycopy calls), providing substantial performance gains across different data sizes. Both implementations produce identical results (verified in benchmark setup). |
There was a problem hiding this comment.
Pull request overview
This PR optimizes the splitAmntFast method by eliminating redundant array allocation and copying operations in the hot path. The optimization removes 4 array operations per layer (2 allocations + 2 copies), achieving 2-3.5x speedup by operating directly on matrix storage rather than extracting/copying columns.
Key Changes
- Inlined group operations directly into matrix storage methods, eliminating temporary array copies
- Added reusable in-place variants (
groupCumSumInPlace,groupDiffInPlace) for cases where original values aren't needed - Created comprehensive benchmarks to measure isolated group operation performance
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| vecxtensions/src/groupDiff.scala | Adds groupDiffInPlace function that computes group differences in-place without allocating result array |
| vecxtensions/src/groupCumSum.scala | Adds groupCumSumInPlace function that computes cumulative sums in-place without allocating result array |
| vecxtensions/src/Tower.scala | Refactors applyGroupCumSumFast and applyGroupDiffFast to operate directly on matrix storage, eliminating 4 array operations per layer |
| benchmark/src/groupOpsBenchmark.scala | Adds comprehensive JMH benchmark for isolated group operations with configurable sizes and group distributions |
| inline def groupDiffInPlace(groups: Array[Int], values: Array[Double]): Unit = | ||
| val n = groups.length | ||
| if n > 0 then | ||
| var i = 0 | ||
| while i < n do | ||
| val g = groups(i) | ||
| var prevValue = 0.0 | ||
| var isFirstInGroup = true | ||
|
|
||
| // Process block of same group, computing differences in-place | ||
| while i < n && groups(i) == g do | ||
| val currentValue = values(i) | ||
| if isFirstInGroup then | ||
| // First element in group gets its own value - no change needed | ||
| isFirstInGroup = false | ||
| else | ||
| values(i) = currentValue - prevValue | ||
| end if | ||
| prevValue = currentValue | ||
| i += 1 | ||
| end while | ||
| end while | ||
| end if | ||
| end groupDiffInPlace No newline at end of file |
There was a problem hiding this comment.
The new groupDiffInPlace function lacks test coverage. Since the repository has comprehensive test coverage for group operations (see groupDiff.test.scala), tests should be added for this new in-place variant to verify:
- It produces the same results as
groupDiffwhen operating on a copy of the input - It correctly handles edge cases (empty arrays, single elements, single groups)
- It properly modifies the input array in place
| inline def groupCumSumInPlace(groups: Array[Int], values: Array[Double]): Unit = | ||
| val n = groups.length | ||
| if n > 0 then | ||
| var i = 0 | ||
| while i < n do | ||
| val g = groups(i) | ||
| var cumSum = 0.0 | ||
|
|
||
| // Process block of same group, computing cumulative sum in-place | ||
| while i < n && groups(i) == g do | ||
| cumSum += values(i) | ||
| values(i) = cumSum | ||
| i += 1 | ||
| end while | ||
| end while | ||
| end if | ||
| end groupCumSumInPlace No newline at end of file |
There was a problem hiding this comment.
The new groupCumSumInPlace function lacks test coverage. Since the repository has comprehensive test coverage for group operations (see groupCumul.test.scala), tests should be added for this new in-place variant to verify:
- It produces the same results as
groupCumSumwhen operating on a copy of the input - It correctly handles edge cases (empty arrays, single elements, single groups)
- It properly modifies the input array in place
| var valuesCopy1: Array[Double] = uninitialized | ||
| var valuesCopy2: Array[Double] = uninitialized | ||
|
|
||
| @Setup(Level.Invocation) |
There was a problem hiding this comment.
The benchmark setup uses Level.Invocation which causes the setup method to run before every benchmark invocation. This adds significant overhead to the measurements and makes the results less accurate. Consider using Level.Iteration instead, which runs setup once per iteration (a group of invocations), providing more accurate timing measurements while still ensuring fresh data for each iteration.
| @Setup(Level.Invocation) | |
| @Setup(Level.Iteration) |
* groupOps * testing * benching * Optimize splitAmntFast by eliminating redundant array copies in group operations (#59) * Initial plan * Optimize group operations: eliminate array copies, add in-place variants - Eliminate unnecessary array copies in applyGroupCumSumFast and applyGroupDiffFast - Add groupCumSumInPlace and groupDiffInPlace for 34-53% performance gains - Optimize group lookups by caching values to avoid redundant array access - Add isolated benchmark for group operations performance testing Performance improvements (10k elements, 100 groups): - groupCumSum: 34% faster with in-place variant (11.6μs → 7.6μs) - groupDiff: 53% faster with in-place variant (10.3μs → 4.9μs) Co-authored-by: Quafadas <24899792+Quafadas@users.noreply.github.com> * Remove ineffective group lookup optimization from code review feedback Code review correctly identified that caching group values in the while condition doesn't reduce array accesses since the array is still accessed on every iteration. Co-authored-by: Quafadas <24899792+Quafadas@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Quafadas <24899792+Quafadas@users.noreply.github.com> * Testing splitAmnt * split works * . * formatting * format * remove days * . * arg * . * seems to pass * . * . * . * . * . * . * . * . * . * benchmark * format * . * . * . * . * format * . * . --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: Quafadas <24899792+Quafadas@users.noreply.github.com>
The
splitAmntFastimplementation was performing 4 array operations per layer (2 allocations + 2 copies) inapplyGroupCumSumFastandapplyGroupDiffFast, defeating the purpose of SIMD optimization.Changes
applyGroupCumSumFastandapplyGroupDiffFastto operate directly on matrix storage instead of extracting columns to temporary arraysgroupCumSumInPlaceandgroupDiffInPlacevariants that modify arrays directly for cases where the original values aren't neededGroupOpsBenchmarkto measure isolated group operation performancePerformance Impact
Full splitAmnt Benchmark Results
Benchmarks comparing
splitAmntOriginalvssplitAmntFast:10,000 losses:
1,000,000 losses:
Isolated Group Operations (10k elements, ~100 groups)
groupCumSum: 34% faster (11.6μs → 7.6μs)groupDiff: 53% faster (10.3μs → 4.9μs)Example
Before (with array copies):
After (direct operation):
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.