Migrate to me.saket.bytesize - #2711
Conversation
(cherry picked from commit 1f9b939)
1f9b939 to
59650ff
Compare
Take over square#2711, which pinned the 2.0.0-beta04 prerelease because every stable bytesize 2.x release is compiled against Kotlin 2.2.0 metadata, which Kotlin 1.9.25 could not read. Now that LeakCanary builds with Kotlin 2.4 (square#2841) that constraint is gone, so this moves to 2.2.1. 2.2.1 also fixes a beta04 bug where sizes below -1000 bytes rendered in scientific notation ("-1.7949673E9 B" rather than "-1.79 GB"), which was reachable through a negative `retainedIncrease`. `Retained.heapSize` is typed as the concrete `DecimalByteSize` rather than the `ByteSize` sealed interface. Shark only ever means decimal bytes, so naming the subtype says that in the signature, and it keeps the value class unboxed for callers that hold onto the static type instead of making every read go through interface dispatch. `shark.api` is regenerated: `shark.ByteSize` is gone, and `Retained`'s mangled names change, so this breaks binary compatibility even for callers that never named `ByteSize`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Take over square#2711, which pinned the 2.0.0-beta04 prerelease because every stable bytesize 2.x release is compiled against Kotlin 2.2.0 metadata, which Kotlin 1.9.25 could not read. Now that LeakCanary builds with Kotlin 2.4 (square#2841) that constraint is gone, so this moves to 2.2.1. 2.2.1 also fixes a beta04 bug where sizes below -1000 bytes rendered in scientific notation ("-1.7949673E9 B" rather than "-1.79 GB"), which was reachable through a negative `retainedIncrease`. `Retained.heapSize` is typed as the concrete `DecimalByteSize` rather than the `ByteSize` sealed interface. Shark only ever means decimal bytes, so naming the subtype says that in the signature, and it keeps the value class unboxed for callers that hold onto the static type instead of making every read go through interface dispatch. `ObjectGrowthDetector`'s two per-object loops sum raw longs rather than using `ByteSize.plus()`, whose parameter is the sealed interface and so boxes both operands. HotSpot scalar replaces those boxes, so on the JVM the operator is free (0.39 bytes/iteration measured over 1M iterations), but ART does not: on an Android 14 arm64 device the operator allocates 32 bytes per iteration and runs the loop 18x slower. End to end over large-dump.hprof that is 9MB of extra garbage per traversal, ~1% of the traversal's total allocation. Small, but this loop was allocation free before the migration and shark runs in the app's own process. `shark.api` is regenerated: `shark.ByteSize` is gone, and `Retained`'s mangled names change, so this breaks binary compatibility even for callers that never named `ByteSize`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
59650ff to
ed79941
Compare
|
🤖 Posted by PY's AI agent. I took this over, rebased it, updated it to bytesize 2.2.1, and drove it to green. All 11 checks pass. Here's everything I found along the way. What I changed on top of @saket's commit
The blocker that's now gonebytesize 2.2.1 ships Kotlin 2.2.0 metadata. On Kotlin 1.9.25 this failed hard — The real finding: nothing in bytesize's arithmetic is allocation-free on ARTThe deleted A value class stays unboxed only while its static type is the value class. Passing one as Bytecode for one
|
| Call site | Allocations |
|---|---|
a + b, a - b, a / b, a < b |
2 each |
a * 2, a / 2 |
2 each |
n.decimalBytes (Int/Long) |
1 |
n.kilobytes |
2 |
I got this wrong twice before getting it right
I initially reported the boxing as harmless. A JVM A/B measurement showed no meaningful difference, so I retracted the concern. That was two mistakes: my first harness only did 5 warmups and its run-to-run spread (10.5 MB across two runs of identical code) was 5× larger than the effect I was trying to resolve; and once fixed, the JVM genuinely shows 0.39 bytes/iteration, because HotSpot's C2 escape analysis scalar-replaces the boxes.
ART does not do this. On a Square Handheld (Android 14, arm64), 1M iterations:
operator += |
raw longs | |
|---|---|---|
| JVM (HotSpot, escape analysis) | 0.39 bytes/iter | 24 bytes total |
| ART | 32.0 bytes/iter | 0.0 bytes/iter |
32 bytes = two 16-byte objects (8-byte header + 8-byte long field). End-to-end over large-dump.hprof (40 MB): +9.03 MB of extra garbage per traversal (+1.07%), ~90× the per-variant noise. 9,030,600 / 32 ≈ 282k loop iterations, which matches the traversal. I'm not claiming the wall-time delta — those ranges overlapped.
The JVM number wasn't wrong, it was measuring the wrong runtime.
The workaround in this PR
ObjectGrowthDetector sums raw longs in the two hot loops rather than using +=, with a comment explaining why. This costs plusExact's overflow check, which is immaterial here: Retained packs heapSize into an Int (heapSize.inWholeBytes.toInt(), documented as "should not exceed Int.MAX_VALUE bytes"), and the old shark.ByteSize used a plain + with no check either.
@saket's suggestion, and why it isn't quite enough
Saket proposed adding a same-type overload. That's exactly the right instinct, but as literally written:
inline operator fun plus(other: DecimalByteSize): DecimalByteSize =
DecimalByteSize(commonPlus(other))it still emits both box-impl calls. The overload resolves fine, but commonPlus is an extension on the BytePrecision interface taking a ByteSize, so both operands get boxed right back inside the body. It has to bypass the interface-typed helpers:
inline operator fun plus(other: DecimalByteSize): DecimalByteSize =
DecimalByteSize(bytes.plusExact(other.bytes))which emits zero. I verified both forms at the bytecode level.
Upstream fix: saket/byte-size#24
I opened saket/byte-size#24 against byte-size. Two independent commits:
- Same-precision overloads of
plus,minus,div,compareToon all three subtypes, with bodies reading the backingLongdirectly. Int/Longreceiver overloads for the twelve unit properties, soretainedSize.decimalBytesstops boxing anInteger. This mirrors what the library already does with itsFloat/Doubleerror stubs, and whatkotlin.time.Durationdoes withInt.seconds/Long.seconds.
Purely additive — metalavaCheckCompatibility passes, api.txt gains 12 methods with zero removals. It ships a test that scans compiled call sites for box-impl and also asserts that mixed-precision call sites still box, so it can't pass vacuously.
Re-measured on ART against that branch published as a snapshot, 2M iterations, with idiomatic += restored in Shark:
| Loop body | bytes/iter |
|---|---|
| raw longs (this PR's workaround) | 0.00 |
total += size.decimalBytes (idiomatic, on the fix) |
0.00 |
| mixed precision (control — still boxes) | 32.00 |
All three produce an identical checksum, so nothing was dead-code-eliminated. Once that lands and is released we can delete the workaround and get overflow checking back for free. I've left a comment in ObjectGrowthDetector pointing at the PR.
Other things worth flagging before merge
- This is a breaking API change.
shark.ByteSizeandshark.bytesare gone. Even callers that never named the type are affected, because value-class signature mangling changes:getHeapSize-UyN4wxk→getHeapSize-mbjz_tw,Retained-5mcd9r4→Retained-X-1A1GI. Migration isshark.ByteSize→me.saket.bytesize.DecimalByteSize,shark.bytes→me.saket.bytesize.decimalBytes. - Retained sizes render differently.
ShortestPathObjectNodeprints${retained.heapSize}, so heap-growth output goes from1 KBto1.5 KBand1 MBto1.05 MB, and gains TB/PB. This is user-visible inshark-cliand in test expectations. - Shark now exposes bytesize as an
apidependency, adding two artifacts to consumers' runtime classpath:me.saket.bytesize:bytesizeand its transitivedev.erikchristensen.javamath2kmp:javamath2kmp:1.1. - The one API 26 CI failure was an emulator boot flake, not a test failure —
adb: device offline→Terminate Emulator→ exit 137, with no test ever executing (2m33s vs 5m52s–7m14s for the passing jobs). It passed on re-run.
Both pre-existing JvmThreadDumpStackTraceEqualityTest failures I saw locally reproduce identically on clean origin/main (a local JDK 21.0.11 Thread.runWith frame) and are unrelated.
Changelog: https://github.com/saket/byte-size/releases/tag/2.0.0-beta02
Would you be comfortable with this, considering it introduces a breaking change for consumers of
:shark?I will mark this PR as ready for review once
byte-sizeis out of beta.