From e8c50ce2a568988d9991454f855b2eaca07c2dda Mon Sep 17 00:00:00 2001 From: Michal Harakal Date: Mon, 10 Aug 2026 10:46:49 +0200 Subject: [PATCH] fix(lang): truthful ownership labels in TensorStorageFactory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit borrowFloatArray promised '(zero-copy)' and returned a Borrowed handle, but a FloatArray has no byte-view in common Kotlin — the implementation re-encoded every float into a private ByteArray and labeled the copy Borrowed. That misrepresents ownership to every consumer of TensorStorage.ownership and to MemoryTracker reports. It is now deprecated (ReplaceWith fromFloatArray) and honestly returns Owned. fromTensorData's KDoc claimed 'the underlying data is borrowed (not copied)' while the dense float/int branches copy via BufferHandleFactory.owned. The contract is now documented per branch: packed Q4_K/Q8_0 borrow genuinely zero-copy (shared packedData), dense arrays convert to owned bytes, everything else materializes. Tests: the acceptance test that enshrined the false promise (ac3_borrowedConstructorDoesNotCopy) now asserts the honest OWNED label; a new ac3 test shows where real borrowing lives (fromRawBytes, with mutation visibility through the shared array); the fromTensorData bridge tests gain ownership assertions plus a packed zero-copy mutation-visibility test. Closes #927 --- CHANGELOG.md | 9 ++++ .../tensor/storage/TensorStorageFactory.kt | 50 ++++++++++++------- .../tensor/storage/AcceptanceCriteriaTest.kt | 20 +++++++- .../tensor/storage/BufferHandleFactoryTest.kt | 17 +++++++ 4 files changed, 76 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fca59a3c2..6fd8fd230 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,15 @@ ## [Unreleased] +### Fixed + +- **`TensorStorageFactory` ownership labels are now truthful.** `borrowFloatArray` re-encoded + the floats into a private byte copy and labeled it `Borrowed` despite its "(zero-copy)" doc — + it is now deprecated (delegating to `fromFloatArray`) and honestly returns `Owned`; + `fromTensorData`'s doc claimed "borrowed (not copied)" while its dense branches copy — the + contract is now documented per branch (packed Q4_K/Q8_0 genuinely borrow zero-copy, dense + arrays convert to owned bytes) and pinned by ownership + mutation-visibility tests. (#927) + ## [0.38.0] - 2026-07-30 ### Added diff --git a/skainet-lang/skainet-lang-core/src/commonMain/kotlin/sk/ainet/lang/tensor/storage/TensorStorageFactory.kt b/skainet-lang/skainet-lang-core/src/commonMain/kotlin/sk/ainet/lang/tensor/storage/TensorStorageFactory.kt index 10ef17e31..b1c1d49ba 100644 --- a/skainet-lang/skainet-lang-core/src/commonMain/kotlin/sk/ainet/lang/tensor/storage/TensorStorageFactory.kt +++ b/skainet-lang/skainet-lang-core/src/commonMain/kotlin/sk/ainet/lang/tensor/storage/TensorStorageFactory.kt @@ -35,25 +35,25 @@ public object TensorStorageFactory { ) /** - * Borrow a FloatArray as dense FLOAT32 storage (zero-copy). + * Convert a FloatArray to dense FLOAT32 storage. + * + * Despite the historical name, this CANNOT borrow: a `FloatArray` has no + * byte-view in common Kotlin, so the floats are re-encoded into a fresh + * little-endian `ByteArray`. The result is therefore an [BufferHandle.Owned] + * buffer — labeling the private copy `Borrowed` (as this method previously + * did) misrepresented ownership to every consumer of + * [TensorStorage.ownership] and to [MemoryTracker] reports. + * + * For genuine zero-copy borrowing, start from bytes: [fromRawBytes] borrows + * the given `ByteArray` without copying. */ - public fun borrowFloatArray(shape: Shape, data: FloatArray): TensorStorage { - val bytes = ByteArray(data.size * 4) - for (i in data.indices) { - val bits = data[i].toRawBits() - val off = i * 4 - bytes[off] = (bits and 0xFF).toByte() - bytes[off + 1] = ((bits shr 8) and 0xFF).toByte() - bytes[off + 2] = ((bits shr 16) and 0xFF).toByte() - bytes[off + 3] = ((bits shr 24) and 0xFF).toByte() - } - return TensorStorage( - shape = shape, - logicalType = LogicalDType.FLOAT32, - encoding = TensorEncoding.Dense(bytesPerElement = 4), - buffer = BufferHandleFactory.borrow(bytes) - ) - } + @Deprecated( + message = "A FloatArray cannot be borrowed as byte storage; this method always copies. " + + "Use fromFloatArray (same behavior, honest name) or fromRawBytes for real borrowing.", + replaceWith = ReplaceWith("fromFloatArray(shape, data)"), + ) + public fun borrowFloatArray(shape: Shape, data: FloatArray): TensorStorage = + fromFloatArray(shape, data) /** * Wrap an IntArray as owned dense INT32 storage (copies the array). @@ -123,7 +123,19 @@ public object TensorStorageFactory { * Bridge: create a [TensorStorage] descriptor from an existing [TensorData]. * * This inspects the concrete TensorData type and builds the appropriate - * storage descriptor. The underlying data is borrowed (not copied). + * storage descriptor. Ownership of the result depends on what the source + * can share: + * + * - **Packed quant data (Q4_K, Q8_0): borrowed, zero-copy.** The + * `packedData` `ByteArray` is shared with the source; mutations are + * visible through both. + * - **Dense float/int data: owned, converted (a copy).** A `FloatArray` / + * `IntArray` has no byte-view in common Kotlin, so the values are + * re-encoded into a fresh little-endian `ByteArray`. + * - **Anything else: owned, materialized (copies).** The tensor is read + * out via [TensorData.copyToFloatArray] and re-encoded. + * + * Check [TensorStorage.ownership] on the result rather than assuming. */ public fun fromTensorData(data: TensorData): TensorStorage { return when (data) { diff --git a/skainet-lang/skainet-lang-core/src/commonTest/kotlin/sk/ainet/lang/tensor/storage/AcceptanceCriteriaTest.kt b/skainet-lang/skainet-lang-core/src/commonTest/kotlin/sk/ainet/lang/tensor/storage/AcceptanceCriteriaTest.kt index 2d712a98f..4acca6ba2 100644 --- a/skainet-lang/skainet-lang-core/src/commonTest/kotlin/sk/ainet/lang/tensor/storage/AcceptanceCriteriaTest.kt +++ b/skainet-lang/skainet-lang-core/src/commonTest/kotlin/sk/ainet/lang/tensor/storage/AcceptanceCriteriaTest.kt @@ -58,10 +58,28 @@ class AcceptanceCriteriaTest { // --- AC3: Tensor views zero-copy, copies explicit --- @Test - fun ac3_borrowedConstructorDoesNotCopy() { + fun ac3_floatArrayConversionIsHonestlyOwned() { + // A FloatArray has no byte-view in common Kotlin: converting it to byte + // storage always copies, so the result must be labeled OWNED. The old + // borrowFloatArray labeled the private copy BORROWED — the lie #927 fixed. val original = floatArrayOf(1f, 2f, 3f) + @Suppress("DEPRECATION") val storage = TensorStorageFactory.borrowFloatArray(Shape(3), original) + assertEquals(Ownership.OWNED, storage.ownership) + } + + @Test + fun ac3_rawByteConstructorGenuinelyBorrows() { + // Real zero-copy borrowing starts from bytes: mutations through the + // source array must be visible through the storage handle. + val bytes = ByteArray(12) + val storage = TensorStorageFactory.fromRawBytes( + Shape(3), LogicalDType.FLOAT32, TensorEncoding.Dense(4), bytes + ) assertEquals(Ownership.BORROWED, storage.ownership) + bytes[0] = 42 + val handle = storage.buffer as BufferHandle.Borrowed + assertEquals(42, handle.data[0]) } @Test diff --git a/skainet-lang/skainet-lang-core/src/commonTest/kotlin/sk/ainet/lang/tensor/storage/BufferHandleFactoryTest.kt b/skainet-lang/skainet-lang-core/src/commonTest/kotlin/sk/ainet/lang/tensor/storage/BufferHandleFactoryTest.kt index aabd2d237..91e75972c 100644 --- a/skainet-lang/skainet-lang-core/src/commonTest/kotlin/sk/ainet/lang/tensor/storage/BufferHandleFactoryTest.kt +++ b/skainet-lang/skainet-lang-core/src/commonTest/kotlin/sk/ainet/lang/tensor/storage/BufferHandleFactoryTest.kt @@ -161,6 +161,9 @@ class TensorStorageFactoryTest { assertEquals(TensorEncoding.Dense(4), storage.encoding) assertEquals(3L, storage.elementCount) assertEquals(12L, storage.physicalBytes) + // Dense float bridges convert (copy) — a FloatArray has no byte-view + // in common Kotlin — so the honest label is OWNED (#927). + assertEquals(Ownership.OWNED, storage.ownership) } @Test @@ -170,6 +173,7 @@ class TensorStorageFactoryTest { assertEquals(LogicalDType.INT32, storage.logicalType) assertEquals(TensorEncoding.Dense(4), storage.encoding) + assertEquals(Ownership.OWNED, storage.ownership) } @Test @@ -184,6 +188,19 @@ class TensorStorageFactoryTest { assertEquals(144L, storage.physicalBytes) } + @Test + fun fromTensorDataPackedBridgeIsZeroCopy() { + // The packed branch borrows the source packedData: a mutation through + // the source must be visible through the storage handle. + val packedData = ByteArray(144) + val tensorData = Q4_KBlockTensorData.fromRawBytes(Shape(256), packedData) + val storage = TensorStorageFactory.fromTensorData(tensorData) + + packedData[7] = 99 + val handle = storage.buffer as BufferHandle.Borrowed + assertEquals(99, handle.data[7]) + } + @Test fun fromTensorDataBridgesQ80TensorData() { val packedData = ByteArray(34) // 1 block of Q8_0