Make same-precision arithmetic and primitive-receiver construction allocation-free - #24
Open
pyricau wants to merge 2 commits into
Open
Make same-precision arithmetic and primitive-receiver construction allocation-free#24pyricau wants to merge 2 commits into
pyricau wants to merge 2 commits into
Conversation
Every operator on `ByteSize` has to accept the sealed interface so that
sizes of different precisions can be mixed. But a `value class` is only
kept unboxed while its static type *is* the value class, so passing one
as a `ByteSize` boxes it. The internal `common*` helpers those operators
delegate to are extensions on `BytePrecision`/`BitPrecision`, which are
interfaces too, so the receiver boxes as well.
The result is that `a + b` allocated twice to perform one `ladd`:
invokestatic DecimalByteSize."box-impl" // argument
invokestatic DecimalByteSize."box-impl" // receiver
invokeinterface BytePrecision.getInWholeBytes:()J // unbox
invokeinterface ByteSize.inWholeBytes:()J // unbox
invokestatic MathKt.plusExact:(JJ)J
`plus`, `minus`, `div` and `compareTo` were all affected, on all three
subtypes.
Add same-precision overloads of those four operators, which the compiler
prefers when both operands have the same concrete type. Their bodies read
the backing `Long` directly instead of going through the interface-typed
helpers — that part matters, since an overload that still delegates to
`commonPlus` boxes both operands right back.
Same-precision arithmetic and comparison now compile to primitive
arithmetic with no allocation and no interface dispatch. Mixing precisions
still goes through `ByteSize` and still boxes, which is unavoidable
without a combinatorial explosion of overloads.
`AllocationFreeCallSitesTest` guards this by scanning compiled call sites
for `box-impl`. It also asserts that mixed-precision call sites *do* still
box, so it cannot pass vacuously if boxing detection ever breaks.
The change is purely additive: `metalavaCheckCompatibility` passes and
api.txt gains 12 methods with no removals.
`Number.decimalBytes` and friends box their receiver: `Integer.valueOf` for an `Int`, `Long.valueOf` for a `Long`. The multiples box twice, since they route through `times(other: Number)`, whose receiver boxes too. So `file.length().binaryBytes` allocated, and `n.kilobytes` allocated twice. Add `Int` and `Long` overloads for the twelve unit properties. This is the same mechanism the library already uses to shadow `Number` with more specific receivers — `Float.decimalBytes` and `Double.decimalBytes` exist as `DeprecationLevel.ERROR` stubs, and their `ReplaceWith` hints already suggest `toInt().decimalBytes` / `toLong().decimalBytes`, which now resolve to these. `kotlin.time.Duration` splits `Int.seconds`/`Long.seconds` for the same reason. Only the integral receivers are added. Their values are exact by construction, so they can call the primary `Long` constructor and skip the fractional-precision check entirely. `Float`/`Double` still need that check and are only meaningful for hand-written literals, so they keep going through `Number`. Overflow behaviour is unchanged: the multiples still use `timesExact`. api.txt is unaffected — these properties are `inline` with `@JvmSynthetic` getters, so metalava does not track them, same as the `Number` overloads they sit beside.
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.
Hey! While migrating LeakCanary/Shark to bytesize, we found that none of the arithmetic on a
ByteSizeis actually allocation-free on Android. This PR fixes that. It's purely additive —metalavaCheckCompatibilitypasses and api.txt gains 12 methods with no removals.What's wrong
A
value classstays unboxed only while its static type is the value class. Every operator onByteSizehas to accept the sealed interface so that precisions can be mixed, so passing a size to one boxes it. And the internalcommon*helpers those operators delegate to are extensions onBytePrecision/BitPrecision, which are interfaces too — so the receiver boxes as well.a + btherefore allocated twice to perform oneladd:Counting allocations per call site on 2.2.1 (
javap -c -p):a + b,a - b,a / b,a < bByteSize, receiver →BytePrecisiona * 2,a / 2Integer.valueOf+ receivern.decimalBytes(Int/Long)Integer.valueOf/Long.valueOfn.kilobytesInteger.valueOf+ receiver, viatimes(Number)DecimalBitSize+instanceofchainByteSize.inWholeBits()'swhenruns on the boxed valueThis is invisible on the JVM because HotSpot's escape analysis scalar-replaces the boxes — I measured 0.39 bytes/iteration there. ART does not do this. On a physical arm64 Android 14 device,
+=in a loop cost a flat 32 bytes and one extra allocation per iteration. In Shark's object-growth traversal that came to ~9 MB of extra garbage per heap traversal, which is why we currently work around it by summing raw longs.What this PR changes
Commit 1 — same-precision operator overloads.
plus,minus,divandcompareTogain overloads taking the concrete type, which Kotlin prefers when both operands match. Their bodies read the backingLongdirectly.That last part is the load-bearing detail. @saket, you suggested in chat:
The overload resolves, but this still emits both
box-implcalls, becausecommonPlusis an extension on theBytePrecisioninterface taking aByteSize— so the operands get boxed right back inside the body. It has to bypass the interface-typed helpers:which emits zero.
Commit 2 —
Int/Longreceiver overloads for the twelve unit properties. This is the same mechanism the library already uses to shadowNumberwith more specific receivers:Float.decimalBytesandDouble.decimalBytesexist asDeprecationLevel.ERRORstubs, and theirReplaceWithhints already saytoInt().decimalBytes/toLong().decimalBytes— which now resolve to non-boxing code.kotlin.time.DurationsplitsInt.seconds/Long.secondsfor exactly this reason.Only integral receivers are added: their values are exact by construction, so they call the primary
Longconstructor and skip the fractional-precision check.Float/Doublestill need that check and are only meaningful for hand-written literals, so they keep going throughNumber. Overflow behaviour is unchanged — the multiples still usetimesExact.The two commits are independent, so feel free to take one and drop the other. Commit 2 is the bigger API-surface call and that's your judgement, not mine.
Verification
AllocationFreeCallSitesTestscans compiled call sites forbox-impl, the static factory Kotlin generates to box a value class. Its name lands in a class file's constant pool if and only if that class boxes a value class somewhere, so its absence proves the call site allocates nothing. The test also asserts that mixed-precision call sites do still box, so it can't pass vacuously if detection ever breaks. I confirmed it fails onmain.Re-measured on ART with this branch published as a snapshot, 2M iterations on a Square Handheld (Android 14, arm64):
total = DecimalByteSize(total.inWholeBytes + size)(Shark's workaround, unchecked+)total += size.decimalBytes(idiomatic, this branch)total += BinaryByteSize(size)(control — mixes precision, still boxes)All three loops produce an identical checksum, so none of them was dead-code-eliminated. The idiomatic form now allocates nothing, which means Shark can drop its workaround and get overflow checking back for free.
The remaining 30 ms vs 6 ms is
plusExact's overflow branch, which the workaround skips entirely by using a plain+. It's ~12 ns/iteration — for Shark that's ~3 ms against a 14.5 s traversal, so it's a trade we're happy to make.Tests pass on JVM, JS (Chrome headless), wasmJs and wasmWasi; all native and JS targets compile. I couldn't run native tests locally — this machine only has the Xcode command line tools, and
linkDebugTestMacosArm64fails the same way onmain.Not addressed
times(Number)/div(Number)still box twice. Fixing it needs primitive overloads forInt/Long/Double/Floatacross three types, and scalar multiplication isn't usually in a hot loop. Happy to add it if you want it.BinaryByteSize.minus(ByteSize)returnsByteSizewhereplusreturnsBinaryByteSize— looks like an oversight, but changing it is binary-breaking so I left it. The newminus(BinaryByteSize)does return the concrete type, which incidentally meansbinA - binBnow infers asBinaryByteSize.errors.kt,Float.decimalBitsandDouble.decimalBitsare declared asDecimalByteSizerather thanDecimalBitSize. Inert, since they'reERROR-level stubs, but you may want to fix it.