perf(integer): back Int and UInt with OverflowingInt by JW-Huang · Pull Request #1064 · amazon-ion/ion-rust

JW-Huang · 2026-10-05T21:06:26Z

Int and UInt now store OverflowingInt directly, the same compact representation Coefficient adopted in #1057.

Type	Before (size / align)	After
`Int`, `UInt`	32 / 16	16 / 8
`Element`	80 / 16	64 / 8

Element now fits in one cache line, and a compile-time assertion keeps it there.

Design note: IntData/UIntData are removed. All integer logic now lives on OverflowingInt, so Int/UInt wrap it directly and only enforce their invariants:

UInt is never negative. Every constructor takes an unsigned magnitude.
Int is never -0. No constructor takes a sign, and neg goes through minus, which maps zero to +0.

Keeping IntData/UIntData as a middle layer would mean every method forwards twice without adding anything. This also matches Coefficient { repr: OverflowingInt }. Happy to restore the layer if you'd prefer it.

Behavior changes (both are fixes):

Padded binary ints are now canonical. An int encoded with extra zero bytes is now == to, and hashes the same as, its minimal encoding. Previously == and cmp disagreed for these values.
u128::try_from(Int) succeeds for values in (i128::MAX, u128::MAX]. It used to return Err even though those values fit.

Other:

BigOrSmall is deleted.
The binary writer, Ion Hash and the primitive TryFrom impls each had their own small/big branches. They now each make one call into OverflowingInt.
Debug is hand-written, so values print as Int(42) / UInt(42).
api/default.txt is regenerated. The concrete From impls cover exactly the types the old blanket impl accepted, and the hand-written Debug impls now appear in the snapshot.

…ul_pow10

popematt

The direction of this PR is good, and I'm looking forward to publishing this in a new release because getting Element down to 64 bytes is really meaningful win. Everything appears to be correct; there are just a few maintainability issues that could be improved.

Earlier I said that OverflowingInt should have all of the necessary functionality for the types that wrap it. I should have been more specific, because I don't think that extends to every byte encoding. (I forgot to explicitly say "minimally necessary set".) I think the rule of thumb is:

OverflowingInt exposes no unsafe operations.
OverflowingInt has a minimal set of operations that are necessary for the wrapper types to do what they need to do. The wrapper types can have richer sets of operations, but those should be built on the minimal set.
Operations that involve two OverflowingInts (self and other) belong to OverflowingInt.
Basic traits like Display, Debug, and Clone belong to OverflowingInt, both because many types should have them and so that the wrapper types can derive them.

Format-specific encodings belong with the format. Methods like magnitude_be_bytes only read the value and can't violate any invariant, so they don't need to be on OverflowingInt. This is what I meant by having as_overflowing_int() so that serialization code can borrow the underlying representation: the encoder borrows magnitude_ref(), produces its bytes at the point of writing, and only allocates when it has to (in the heap arm, or at a public API that returns a Vec<u8>).

Relatedly, where a wrapper enforces its invariant on a raw representation, it would be good to keep that invariant enforcement in only one place. In this case, we don't have complicated constructor logic, so we can just route all construction through a TryFrom impl. I've left the specifics as inline comments.

This doesn't need to block the PR. If you'd rather make the encoder changes in a follow-up, that's fine.

popematt · 2026-10-05T23:19:34Z

-            return self.clone();
+    /// Decodes an unsigned magnitude. Leading zero bytes don't affect the result, and an empty
+    /// slice is zero.
+    pub(crate) fn from_unsigned_bytes(bytes: &[u8], order: ByteOrder) -> Self {


Both callers pass a constant, and none of the other byte methods take a ByteOrder. Can you split from_unsigned_bytes into from_unsigned_le_bytes and from_unsigned_be_bytes and delete the enum so that we're not introducing unnecessary branching?

popematt · 2026-10-05T23:47:16Z

+        let mut be = value.as_overflowing_int().magnitude_be_bytes();
+        Self::write_sign_and_magnitude(sink, &mut be, value.is_negative())


Within the crate, I think we should use magnitude_ref() so that we're borrowing the representation as much as possible/until the last possible moment when we can no longer borrow it. This will make it easier in future to eliminate some of the unnecessary allocation in the writer.

Suggested change

let mut be = value.as_overflowing_int().magnitude_be_bytes();

Self::write_sign_and_magnitude(sink, &mut be, value.is_negative())

let magnitude = value.as_overflowing_int().magnitude_ref();

match magnitude {

Magnitude::Small(small) => {

let mut be = small.to_be_bytes();

Self::write_sign_and_magnitude(sink, &mut be, value.is_negative())

}

Magnitude::Big(big) => cold_path! {{

let mut be = big.to_bytes_be();

Self::write_sign_and_magnitude(sink, &mut be, value.is_negative())

}}

}

popematt · 2026-10-06T01:26:22Z

-        self.data.neg().into()
+        Int {
+            repr: self.repr.minus(),
+        }


This takes self by value, but minus(&self) → with_sign(&self) clones the heap BigUint and then drops the original. Every decoder builds negative values with .neg(), so with the 126-bit inline limit, reading i128::MIN now allocates twice. minus has no other non-test callers, so can you change it (and with_sign) to take self? Then the heap case moves the Box instead of cloning it, and the inline case becomes a single bit flip in the offset-0 word. That's a safe write to a Copy union field, and the value stays canonical because the magnitude doesn't change. abs(&self) would need an explicit self.clone().with_sign(...), which is fine.

This method body would then become:

self.repr.minus(); self

Also who named it minus() when negate() would be a much better name?! (It was me 🤦🏻‍♂️)

popematt · 2026-10-06T22:29:52Z

+    /// The value as the primitive integer `T` when it fits.
+    pub(crate) fn to_primitive<T: TryFrom<i128> + TryFrom<u128>>(&self) -> Option<T> {
+        match self.as_i128() {
+            Some(value) => T::try_from(value).ok(),
+            // Above `i128::MAX` (or below `i128::MIN`, where `as_u128` is `None`).
+            None => self.as_u128().and_then(|value| T::try_from(value).ok()),
+        }
+    }


as_i128 and as_u128 already exist to support conversions to primitives, so I don't think OverflowingInt needs a generic to_primitive as well. The wrappers can use as_i128() for conversions to signed types, and as_u128() for conversion to unsigned types, and then narrow from there as needed.

popematt · 2026-10-06T22:31:12Z

-    /// arms, which is what lets the scaling guards and byte-length calculations
-    /// avoid a conversion.
+    /// arms, which is what lets the scaling guards avoid a conversion.


Was there a change in this PR that affects the correctness of the comment or was this something that was already inaccurate?

popematt · 2026-10-06T23:54:28Z

-        } else {
-            magnitude
+        Ok(Int {
+            repr: value.clone(),


We should delete this impl and replace it with an owned TryFrom<OverflowingInt> for Int (and the equivalent for UInt). Those become the single guarded entry point from a raw representation for each wrapper.

Hiding a clone() inside a TryFrom is a reasonable convenience for public API (std does the same thing with From<&str> for String), but this one is a crate-private conversion between a wrapper and its representation, and it makes the cost invisible to performance-sensitive internal code. Its only caller is Coefficient::as_int(), and every caller of as_int() ends up cloning when it didn't need to:

TryFrom<Coefficient> for Int already owns the coefficient, but clones the repr anyway

TryFrom<&Coefficient> for Int clones the coefficient and then clones the repr again

The binary decimal writer (binary/decimal.rs:62-65) clones the coefficient into an Int for every non-zero decimal it writes, only to borrow it in DecodedInt::write

Decimal → BigDecimal (decimal/mod.rs:483) clones only to reject -0

With only the owned impl, a clone has to be spelled out at the call site, and the callers that own the value move it instead. Those call sites aren't in this diff, so I'm fine with cleaning them up in a follow-up. For now, removing this impl only means changing as_int() to Int::try_from(self.repr.clone()).ok(), which at least puts the clone where it can be seen. (The public TryFrom<&Int> for UInt and TryFrom<&Coefficient> impls should stay; removing them would be a breaking change.)

Since the owned impls are pub(crate), you might consider type Error = OverflowingInt so that a failed conversion hands the value back. The public Coefficient impls can map that to an IonError.

popematt · 2026-10-07T00:06:46Z

 impl Display for Int {
    fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), std::fmt::Error> {
-        write!(f, "{}", self.data)
+        write!(f, "{}", self.repr)


minor suggestion—we can avoid creating an unnecessary fmt::Arguments (created by the write! macro) in this method when all we really need to do is delegate to the Display impl for OverflowingInt:

Suggested change

write!(f, "{}", self.repr)

Display::fmt(&self.repr, f)

(This also applies to impl Display for UInt.)

popematt · 2026-10-07T00:14:38Z

+    }
+
+    #[test]
+    fn int_to_u128_above_i128_max() {


This needs some negative cases. Rejecting a negative value for an unsigned target depends on as_u128() returning None, and no test checks that for a value too large to fit in an i128. Can you turn this into an rstest table that includes cases like u128::try_from(Int::from(u128::MAX).neg()), i128::try_from(Int::from(i128::MIN)), and i128::try_from(Int::from(i128::MIN).neg())?

popematt · 2026-10-07T00:21:14Z

+    }
+
+    #[test]
+    fn padded_bytes_equal_unpadded() {


This only covers the little-endian constructors, but if there is a regression it could show up on the binary reader path (which uses the big-endian constructor). Can you add a big-endian version of this test please?

popematt · 2026-10-07T00:23:27Z


        // Big fractional coefficient (>18 digits) is rejected
-        let big_coefficient: Int = Int::from(i128::MAX).data.mul(4).into();
+        let big_coefficient = Int::from(u128::MAX);


u128::MAX fits in a u128, so this no longer reaches the .as_u128().unwrap_or(u128::MAX) fallback at timestamp.rs:1280-1284. The test still passes, but only because of the precision check. Can you build a coefficient wider than u128 instead? Int::from_le_signed_bytes with 16 zero bytes followed by 0x02 gives you 2^129.

…ngInt` docs `with_sign` now rewrites the sign bit of an owned value in place, so the module's immutability note and the `HeapValue::new` "only place that computes `meta`" note no longer held. Both now name `with_sign` and state that it touches only bit 62.

Will Huang added 7 commits October 5, 2026 10:42

refactor(integer): remove unused IntData/UIntData operators and helpers

d475fe1

feat(integer): add byte and radix codecs to OverflowingInt

6642f5a

perf(integer): back Int and UInt with OverflowingInt

f77621c

chore(integer): drop BigOrSmall now that nothing backs onto it

ed4bde7

refactor(integer): collapse remaining duplicate integer conversions

ef231fb

refactor(integer): drop OverflowingInt's dead-code allow and unused m…

024af67

…ul_pow10

test(integer): pin Element layout and hand-write Int/UInt Debug

568344d

JW-Huang marked this pull request as ready for review October 5, 2026 21:09

popematt reviewed Oct 7, 2026

View reviewed changes

Will Huang and others added 3 commits October 8, 2026 12:18

refactor(integer): address review feedback

1cdb0cd

Merge branch 'main' into int-compact

ddc3d86

popematt approved these changes Oct 8, 2026

View reviewed changes

popematt merged commit 11e186a into amazon-ion:main Oct 8, 2026
19 checks passed

Navigation Menu

Repository navigation

perf(integer): back Int and UInt with OverflowingInt - #1064

perf(integer): back Int and UInt with OverflowingInt#1064
popematt merged 10 commits into
amazon-ion:mainfrom
JW-Huang:int-compact

JW-Huang commented Oct 5, 2026

Uh oh!

popematt left a comment

Uh oh!

popematt Oct 5, 2026

Uh oh!

popematt Oct 5, 2026

Uh oh!

popematt Oct 6, 2026

Uh oh!

popematt Oct 6, 2026

Uh oh!

popematt Oct 6, 2026

Uh oh!

popematt Oct 6, 2026

Uh oh!

popematt Oct 7, 2026

Uh oh!

popematt Oct 7, 2026

Uh oh!

popematt Oct 7, 2026

Uh oh!

popematt Oct 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		let mut be = value.as_overflowing_int().magnitude_be_bytes();
		Self::write_sign_and_magnitude(sink, &mut be, value.is_negative())

-        let mut be = value.as_overflowing_int().magnitude_be_bytes();
-        Self::write_sign_and_magnitude(sink, &mut be, value.is_negative())
+        let magnitude = value.as_overflowing_int().magnitude_ref();
+        match magnitude {
+            Magnitude::Small(small) => {
+                let mut be = small.to_be_bytes();
+                Self::write_sign_and_magnitude(sink, &mut be, value.is_negative())
+            }
+            Magnitude::Big(big) => cold_path! {{
+                let mut be = big.to_bytes_be();
+                Self::write_sign_and_magnitude(sink, &mut be, value.is_negative())
+            }}
+        }

Conversation

JW-Huang commented Oct 5, 2026

Uh oh!

popematt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants