perf(types): drop two per-value allocations on the binary write path - #556
Open
polyglotAI-bot wants to merge 4 commits into
Open
Conversation
AbstractBigIntegerType.WriteValue allocated BigInteger.ToByteArray() plus a byte[Size] destination per value; it now writes into a stack buffer with BigInteger.TryWriteBytes(isUnsigned: !Signed), mirroring the read side and DecimalType.WriteBigInteger. The multidimensional blit fast path was gated on the concrete leaf class, so a leaf behind a wire-transparent wrapper (LowCardinality, SimpleAggregateFunction, Object) fell back to the boxing walk although it writes identical bytes. The gate now strips those wrappers; Nullable is deliberately not stripped. Wire output is unchanged in both cases. Fixes: #553
polyglotAI-bot
requested review from
alex-clickhouse and
mzitnik
as code owners
August 15, 2026 12:32
Collaborator
|
Stack this one on 550 too please. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…locations' into polyglot/cs553-bigint-write-and-wrapped-blit
… gate Now that this PR is stacked on #550, the private UnwrapTransparent helper in MultiDimArrayHelper duplicates TransparentWrapper.Unwrap introduced there, with identical semantics (LowCardinality / SimpleAggregateFunction / Object stripped, Nullable deliberately kept). Drop the local copy and call the shared helper. No behaviour change.
polyglotAI-bot
changed the base branch from
main
to
polyglot/cs549-residual-value-allocations
August 15, 2026 12:46
Collaborator
Author
|
Done — retargeted onto One follow-on from stacking: the private Full unit suite on the stacked base: 10 800 passed, 0 failed (net10.0). |
alex-clickhouse
approved these changes
Aug 16, 2026
The typed `ITypedReader<Int128>`/`ITypedReader<UInt128>` readers exist so a POCO property of the native CLR type avoids `BigInteger`'s heap array. Both decoded through `ReadBytes(int)`, which returns a fresh `byte[16]` per value, so the path allocated 40 bytes per value on 64-bit — the allocation the typed reader was added to remove. Read into `stackalloc byte[Size]` via the existing `ReadBytes(Span<byte>)` overload instead, the same shape the base `AbstractBigIntegerType` already uses. Decoded values are unchanged. `Int256`/`UInt256` have no native CLR counterpart, so they only have the `BigInteger` reader and were already allocation-free. Test: `NativeWideIntegerRead_OfManyValues_ShouldNotAllocate`, the read-side mirror of the existing write-side guard. It measures 40 B/value before this change and 0 after. Co-Authored-By: Claude <noreply@anthropic.com>
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.
Description
Fixes #553.
Two per-value heap allocations on the RowBinary write path. Neither changes a byte on the wire — the values were already correct, the garbage was not.
AbstractBigIntegerType.WriteValueallocatedBigInteger.ToByteArray()plus anew byte[Size]destination for everyInt128/UInt128/Int256/UInt256value. The read side of the same class already decodes from astackallocspan; only the write side was left behind.TryGetBlittableElementSize. A leaf behind a wire-transparent wrapper —LowCardinality(Int32),SimpleAggregateFunction(any, Int32)— is a different class, so the whole array fell back toWriteAxis, boxing every element throughArray.GetValue(int[]). Those wrappers delegateWritestraight to the underlying type, so the fallback bought nothing.Changes
Types/AbstractBigIntegerType.cs— write intostackalloc byte[Size]withBigInteger.TryWriteBytes(buffer, out written, isUnsigned: !Signed), thenSlice(written).Fill(sign < 0 ? 0xFF : 0x00).Sizeis 16 or 32, so the stack buffer is bounded. Same shape asDecimalType.WriteBigInteger(Reduce allocations in DecimalType.WriteBigInteger #432).isUnsignedreplaces the old "trim BigInteger's trailing sign byte" step and reports the same count in theOverflowExceptionmessage; theArgumentExceptionfor a negative value on an unsigned type is unchanged.Types/MultiDimArrayHelper.cs— the blit gate now looks the leaf up throughTransparentWrapper.Unwrap(LowCardinality,SimpleAggregateFunction,Object).Nullableis deliberately not unwrapped: it prefixes a per-element null marker byte, so it is not wire-transparent and keeps the per-element path. Only the gate looks through the wrapper; the slow path still receives the original leaf, and the CLR-element-type check is unchanged.ClickHouse.Driver.Benchmark/MultidimArrayInsert.cs— newLeafparameter so the existing blit-vs-boxing comparison also covers the wrapped leaves.ClickHouse.Driver.Benchmark/WideIntegerInsert.cs— new benchmark for the wide-integer write path, withInt64as the baseline.changelog.d/553-...improvements.md.Test
Tests/Misc/SerialisationTests.cs— write-side mirror of the existing read-side sign tests: a parametrized boundary set (zero, ±1,-2, signed min/max, unsigned max,2^(bits-1)unsigned) asserting the exact little-endian two's-complement bytes including the sign/zero extension fill, plus the preservedArgumentException(negative on unsigned) andOverflowExceptionon both the positive and the negative overflow boundary with the exact message, plus an allocation test.Tests/Types/MultiDimArrayHelperTests.cs— the wrapped leaves are added to the existing multidim-vs-jagged equivalence source (the jagged form takes the untouched boxing path, so it is an independent oracle), plus an allocation test for both wrappers.LowCardinality(Nullable(Float64))with adouble?[,]pins that unwrapping stops atNullable: addingNullableTypeto the unwrap set makes that case fail (verified by mutation).Measured with
GC.GetAllocatedBytesForCurrentThread(net10.0), before → after:Int128/UInt128writeInt256/UInt256writeint[500,500]intoArray(Array(LowCardinality(Int32)))int[500,500]intoArray(Array(SimpleAggregateFunction(any, Int32)))int?[2,3]intoArray(Array(Nullable(Int32)))(control)BenchmarkDotNet (
BENCH_WARMUP=1 BENCH_ITERATIONS=8 BENCH_LAUNCHES=1,ENGINE Nulltable, net10.0):WideIntegerInsert— 100 000 rows, Allocated per insert:Int128: 7841 KB → 28.9 KBInt256: 9404 KB → 28.6 KBUInt128: 7841 KB → 28.6 KBUInt256: 9404 KB → 28.6 KBInt64(baseline, untouched): 28.8 KB → 28.8 KBMultidimArrayInsert— 100 rows ofint[100,100],MultidimBlit, Allocated per insert (and ratio against the jagged boxing baseline in the same run):Int32(untouched): 29.4 KB → 29.4 KB, 0.68× meanLowCardinality(Int32): 23 470 KB → 30.5 KB, 1.10× → 0.79× meanSimpleAggregateFunction(any, Int32): 23 471 KB → 30.3 KB, 1.06× → 0.70× meanMeans are network-bound and noisy at this iteration count; the Allocated column is the signal.
Full unit suite on net10.0: 10 525 passed, 0 failed. No existing test was changed or removed.
Pre-PR validation gate
main, pass here)dotnet run scripts/changelog.cs -- --checkOK)AGENTS.mdNote
Base:
polyglot/cs549-residual-value-allocations(#550), per review request. #550 is itself stacked on #499, so this is the third PR in that perf stack; the diff above is only this PR's own change.Stacking made the duplicate unwrap helper unnecessary:
MultiDimArrayHelpernow callsTransparentWrapper.Unwrapfrom #550 instead of keeping a private copy with the same semantics (LowCardinality/SimpleAggregateFunction/Objectstripped,Nullabledeliberately kept). Full unit suite green on the stacked base: 10 800 passed, 0 failed (net10.0).