perf: build scalar-on-left results in a single pass (#258) - #351
Merged
Merged
Conversation
The 32 scalar-on-left operators in `_Arithmetic.kt` each copied the whole operand with `deepCopy()` and then walked the copy a second time to apply the operation. The fix named in the issue - mirroring `if (consistent) copy() else deepCopy()` from `_ArithmeticNDArray.kt` - would not have helped: `deepCopy()` already branches on `consistent`, and in that branch it does exactly what `copy()` does (`data.copyOf()`, offset 0, default strides), so the two are interchangeable there. The cost was the redundant pass, not the choice of copy. Each operator now fills the result buffer in one traversal, reading the source's backing array directly: sequentially when the source is `consistent`, otherwise through a row-major odometer over offset/strides. Both paths are primitive and boxing-free, so views no longer pay for the boxed `Iterator<T>` that `deepCopy()` used. Measured on 1000x1000 arrays, old vs new in the same JVM: contiguous source 1.4x (ComplexFloat) .. 7.0x (Byte) transposed view 1.7x (ComplexFloat) .. 4.6x (Int.minus) Results are bit-identical: the lambdas keep each operand on the side the previous implementation used, and Byte/Short keep their narrowing conversions. Public API is unchanged; the new per-dtype kernels are private. The 32 `as NDArray` unchecked casts are gone with them, so a third-party `MultiArray` no longer fails with a `ClassCastException`. Adds `ScalarArithmeticTest`, the first test coverage these operators have: all 8 dtypes x 4 ops, contiguous sources and transposed/offset/stepped views, detachment from the source buffer, result layout, empty arrays, Byte/Short truncation, and complex division by zero.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Qualify the inaccurate deepCopy() rationale before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Optimizes scalar-on-left ndarray arithmetic with single-pass, primitive kernels for contiguous and non-contiguous layouts.
Changes:
- Adds per-dtype scalar arithmetic kernels.
- Adds tests for views, layouts, detachment, empty arrays, truncation, and complex division.
File summaries
| File | Summary |
|---|---|
multik-core/src/commonTest/kotlin/org/jetbrains/kotlinx/multik/ndarray/operation/ScalarArithmeticTest.kt |
Tests arithmetic correctness, views, layouts, detachment, and edge cases. |
multik-core/src/commonMain/kotlin/org/jetbrains/kotlinx/multik/ndarray/operations/_Arithmetic.kt |
Implements optimized single-pass scalar arithmetic kernels. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+283
to
+284
| // on every backend, with no boxing on either path. Neither the public `map` nor [NDArray.deepCopy] | ||
| // can be reused here: both go through `Iterator<T>`, which boxes every element. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #258.
The 32 scalar-on-left operators copied the operand with
deepCopy()and then walked the copy again. They now fill the result buffer in one traversal, reading the source's backing array directly: sequentially when the source isconsistent, otherwise through a row-major odometer overoffset/strides. Both paths are primitive and boxing-free.The fix named in the issue would not have helped:
deepCopy()already branches onconsistent, and in that branch it does exactly whatcopy()does. The cost was the redundant pass.Old vs new, same JVM, 1000x1000: contiguous 1.4x (ComplexFloat) - 7.0x (Byte); transposed view 1.7x - 4.6x.
Results are bit-identical. Public API unchanged; the per-dtype kernels are private.
Testing
:multik-core:jvmTest,:multik-kotlin:jvmTest,:multik-core:macosArm64Test,apiCheck korroCheck- all pass,multik-core.apiunchanged. JS/WASM/iOS test sources compile (their test tasks are skipped in this repo, #247).New
ScalarArithmeticTest(19 tests, first coverage these operators have): 8 dtypes x 4 ops, contiguous and transposed/offset/stepped views, detachment from the source buffer, result layout, empty arrays, Byte/Short truncation, complex division by zero.Checklist