diff --git a/CHANGELOG.md b/CHANGELOG.md index 78ba9816c..bc344eadb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,12 @@ ### 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) - **`TensorStorage` transfer API can materialize its own placements.** `copyMaterialize()` threw for `Aliased` handles (now resolved directly, producing an independent owned copy of the slice) and for `FileBacked` — meaning `copyToHost()` could not bring `MMAP_WEIGHTS` 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