a bit optimize four-digit chunks in integer formatting

[chirizxc]

View all comments

GodBolt

Number of asm instructions has decreased (78 -> 59)

The current upstream version contains:

movabs  rax, 9999999999999999
cmp     rsi, rax
ja      .LBB0_2

(if n > 9_999_999_999_999_999 -> go to panic)

But this function isn't part of the public API, so I suppose we can use assert_unchecked(n < 10_000_000_000_000_000);

Similarly, write_quad has been moved to a separate fn to reduce duplication slightly

[rustbot]

Some changes occurred in integer formatting

cc @tgross35

[rustbot]

r? @clarfonthey

rustbot has assigned @clarfonthey.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from Darksonn, JohnTitor, Mark-Simulacrum, clarfonthey, jhpratt

[clarfonthey]

Haven't reviewed the code yet, but have you actually benchmarked this other than checking the generated assembly? Because number of instructions isn't really a metric we care about, just speed; we have optimize_for_size separately for that.

[chirizxc]

Haven't reviewed the code yet, but have you actually benchmarked this other than checking the generated assembly? Because number of instructions isn't really a metric we care about, just speed; we have optimize_for_size separately for that.

Yes, I'll send results a little later

[chirizxc]

Haven't reviewed the code yet, but have you actually benchmarked this other than checking the generated assembly? Because number of instructions isn't really a metric we care about, just speed; we have optimize_for_size separately for that.

Cargo.toml:

[package]
name = "bench"
version = "0.1.0"
edition = "2024"

[dependencies]

[dev-dependencies]
divan = "0.1.21"

[[bench]]
name = "enc_16lsd"
harness = false

[profile.bench]
lto = "fat"
codegen-units = 1

src/main.rs:

fn main() {}

benches/enc_16lsd.rs:

use core::{hint::black_box, mem::MaybeUninit};

// The string of all two-digit numbers in range 00..99 is used as a lookup table.
static DECIMAL_PAIRS: &[u8; 200] = b"\
      0001020304050607080910111213141516171819\
      2021222324252627282930313233343536373839\
      4041424344454647484950515253545556575859\
      6061626364656667686970717273747576777879\
      8081828384858687888990919293949596979899";

/// Encodes the 16 least-significant decimals of n into `buf[OFFSET .. OFFSET + 16 ]`.
fn enc_16lsd<const OFFSET: usize>(buf: &mut [MaybeUninit<u8>], n: u64) {
    // Consume the least-significant decimals from a working copy.
    let mut remain = n;

    // Format per four digits from the lookup table.
    for quad_index in (1..4).rev() {
        // pull two pairs
        let quad = remain % 1_00_00;
        remain /= 1_00_00;
        let pair1 = (quad / 100) as usize;
        let pair2 = (quad % 100) as usize;
        buf[quad_index * 4 + OFFSET + 0].write(DECIMAL_PAIRS[pair1 * 2 + 0]);
        buf[quad_index * 4 + OFFSET + 1].write(DECIMAL_PAIRS[pair1 * 2 + 1]);
        buf[quad_index * 4 + OFFSET + 2].write(DECIMAL_PAIRS[pair2 * 2 + 0]);
        buf[quad_index * 4 + OFFSET + 3].write(DECIMAL_PAIRS[pair2 * 2 + 1]);
    }

    // final two pairs
    let pair1 = (remain / 100) as usize;
    let pair2 = (remain % 100) as usize;
    buf[OFFSET + 0].write(DECIMAL_PAIRS[pair1 * 2 + 0]);
    buf[OFFSET + 1].write(DECIMAL_PAIRS[pair1 * 2 + 1]);
    buf[OFFSET + 2].write(DECIMAL_PAIRS[pair2 * 2 + 0]);
    buf[OFFSET + 3].write(DECIMAL_PAIRS[pair2 * 2 + 1]);
}

#[inline(always)]
unsafe fn write_quad(buf: &mut [MaybeUninit<u8>], offset: usize, quad: u64) {
    // SAFETY: These are this function's caller-provided invariants.
    unsafe {
        core::hint::assert_unchecked(quad < 10_000);
        core::hint::assert_unchecked(offset <= buf.len() - 4);
    }

    // For the documented range, ceil(2^19 / 100) gives an exact quotiet.
    const DIV100_SHIFT: u32 = 19;
    const DIV100_RECIPROCAL: u32 = (1 << DIV100_SHIFT) / 100 + 1;

    let quad = quad as u32;
    let high = (quad * DIV100_RECIPROCAL) >> DIV100_SHIFT;
    let low = quad - high * 100;
    let high = high as usize;
    let low = low as usize;

    // SAFETY: `high` and `low` are below 100 because quad is below 10_000. The
    // destination has four bytes by the precondition, and the two source pairs
    // are disjoint from it because `DECIMAL_PAIRS` is static RO storage.
    unsafe {
        let pairs = DECIMAL_PAIRS.as_ptr();
        let dst = buf.as_mut_ptr().add(offset).cast::<u8>();
        core::ptr::copy_nonoverlapping(pairs.add(high * 2), dst, 2);
        core::ptr::copy_nonoverlapping(pairs.add(low * 2), dst.add(2), 2);
    }
}

/// Encodes the 16 least-significant decimals of n into `buf[OFFSET .. OFFSET + 16 ]`.
fn enc_16lsd_pr<const OFFSET: usize>(buf: &mut [MaybeUninit<u8>], n: u64) {
    // SAFETY: Every caller passes a remainder produced by division by 10^16,
    // and every used `OFFSET` specialization reserves sixteen bytes in `buf`.
    unsafe {
        core::hint::assert_unchecked(n < 10_000_000_000_000_000);
        core::hint::assert_unchecked(OFFSET <= buf.len() - 16);
    }

    // Peel four digits at a time from right to left (12345678 -> 1234 | 5678).
    // Since 10_000 is constant, LLVM replaces each division with multiply or shift.
    let mut remain = n;

    for quad_index in (1..4).rev() {
        // pull two pairs
        let quad = remain % 1_00_00;
        remain /= 1_00_00;
        // SAFETY: modulo bounds quad; OFFSET and quad_index select one of the
        // four non-overlapping four-byte regions proven in bounds above.
        unsafe { write_quad(buf, quad_index * 4 + OFFSET, quad) };
    }

    // SAFETY: OFFSET starts the first four-byte region proven in bounds above.
    unsafe { write_quad(buf, OFFSET, remain) };
}

fn inputs() -> impl Iterator<Item = u64> {
    (0..10_000).map(|n| n * 1_000_100_010_001)
}

#[divan::bench(sample_count = 1_000)]
fn upstream(bencher: divan::Bencher) {
    bencher.bench_local(|| {
        let mut checksum = 0_u8;
        for n in inputs() {
            let mut buf = [MaybeUninit::uninit(); 16];
            enc_16lsd::<0>(&mut buf, black_box(n));
            checksum ^= unsafe { buf[black_box((n as usize) & 15)].assume_init() };
        }
        black_box(checksum)
    });
}

#[divan::bench(sample_count = 1_000)]
fn pr_159130(bencher: divan::Bencher) {
    bencher.bench_local(|| {
        let mut checksum = 0_u8;
        for n in inputs() {
            let mut buf = [MaybeUninit::uninit(); 16];
            enc_16lsd_pr::<0>(&mut buf, black_box(n));
            checksum ^= unsafe { buf[black_box((n as usize) & 15)].assume_init() };
        }
        black_box(checksum)
    });
}

fn main() {
    // Run registered benchmarks.
    divan::main();
}

cargo bench --bench enc_16lsd

❯ cargo bench --bench enc_16lsd                                                                                                                
   Compiling bench v0.1.0 (C:\Users\chiri\Desktop\xxx)                                                                                         
    Finished `bench` profile [optimized] target(s) in 8.36s                                                                                   
     Running benches\enc_16lsd.rs (target\release\deps\enc_16lsd-0982b2e585327d31.exe)
Timer precision: 100 ns
enc_16lsd     fastest       │ slowest       │ median        │ mean          │ samples │ iters
├─ pr_159130  36.89 µs      │ 79.69 µs      │ 37.09 µs      │ 38.05 µs      │ 1000    │ 1000
╰─ upstream   53.59 µs      │ 122.3 µs      │ 54.29 µs      │ 59.22 µs      │ 1000    │ 1000

❯ cargo bench --bench enc_16lsd                                                                                                                
    Finished `bench` profile [optimized] target(s) in 0.04s                                                                                    
     Running benches\enc_16lsd.rs (target\release\deps\enc_16lsd-0982b2e585327d31.exe)
Timer precision: 100 ns
enc_16lsd     fastest       │ slowest       │ median        │ mean          │ samples │ iters
├─ pr_159130  36.89 µs      │ 79.49 µs      │ 37.19 µs      │ 38.91 µs      │ 1000    │ 1000
╰─ upstream   53.49 µs      │ 135.4 µs      │ 54.19 µs      │ 56.21 µs      │ 1000    │ 1000

❯ cargo bench --bench enc_16lsd                                                                                                                
    Finished `bench` profile [optimized] target(s) in 0.03s                                                                                    
     Running benches\enc_16lsd.rs (target\release\deps\enc_16lsd-0982b2e585327d31.exe)
Timer precision: 100 ns
enc_16lsd     fastest       │ slowest       │ median        │ mean          │ samples │ iters
├─ pr_159130  36.89 µs      │ 105.2 µs      │ 37.09 µs      │ 38.4 µs       │ 1000    │ 1000
╰─ upstream   53.59 µs      │ 90.39 µs      │ 54.09 µs      │ 54.94 µs      │ 1000    │ 1000

❯ cargo bench --bench enc_16lsd                                                                                                                
    Finished `bench` profile [optimized] target(s) in 0.03s                                                                                    
     Running benches\enc_16lsd.rs (target\release\deps\enc_16lsd-0982b2e585327d31.exe)
Timer precision: 100 ns
enc_16lsd     fastest       │ slowest       │ median        │ mean          │ samples │ iters
├─ pr_159130  36.89 µs      │ 88.89 µs      │ 37.09 µs      │ 38.63 µs      │ 1000    │ 1000
╰─ upstream   53.49 µs      │ 112 µs        │ 54.19 µs      │ 57.01 µs      │ 1000    │ 1000

~\Desktop\xxx on  HEAD via 🦀 v1.99.0-nightly 
❯ 

[clarfonthey]

A few minor comments; apologies for taking so long to get to this. The code looks good to me and I can understand why it would be a strict improvement. Would like to run a compiler perf run after you've made your updates just to make sure there are no regressions there, then should be fine merging this.

Based upon the reduced instruction count, it does seem that the assert_unchecked calls are having a positive effect.

[clarfonthey]

Honestly was thinking maybe this would be worded better as write_copy_of_slice to avoid the raw pointer arithmetic, but I don't think it matters too much.

View changes since the review

[chirizxc]

diff --git a/library/core/src/fmt/num.rs b/library/core/src/fmt/num.rs
index d703b817b5a..f94e5433a51 100644
--- a/library/core/src/fmt/num.rs
+++ b/library/core/src/fmt/num.rs
@@ -692,11 +692,12 @@ unsafe fn _fmt_inner(self, buf: &mut [MaybeUninit<u8>]) -> usize {
 
             let quad = remain % 1_00_00;
             remain /= 1_00_00;
-            // SAFETY: quad is a remainder modulo 10_000. The offset checks
-            // above reserve exactly four bytes in buf.
-            unsafe {
-                write_quad(buf.get_unchecked_mut(offset..offset + 4), quad);
-            }
+
+            write_quad(
+                // SAFETY: `offset >= 4` was asserted above.
+                unsafe { buf.get_unchecked_mut(offset..offset + 4) },
+                quad,
+            );
         }
 
         // Format per two digits from the lookup table.
@@ -813,12 +814,8 @@ pub fn format_into(self, buf: &mut NumBuffer<Self>) -> &str {
 }
 
 /// Writes `quad` as exactly four digits (for example: `42` becomes `"0042"`).
-///
-/// # Safety
-///
-/// `quad` must be below 10_000 and `buf` must contain exactly four bytes.
 #[inline(always)]
-unsafe fn write_quad(buf: &mut [MaybeUninit<u8>], quad: u64) {
+fn write_quad(buf: &mut [MaybeUninit<u8>], quad: u64) {
     // SAFETY: These are this function's caller-provided invariants.
     unsafe {
         core::hint::assert_unchecked(quad < 10_000);
@@ -834,15 +831,10 @@ unsafe fn write_quad(buf: &mut [MaybeUninit<u8>], quad: u64) {
     let low = low as usize;
 
     // SAFETY: `high` and `low` are below 100 because `quad` is below 10_000.
-    // The destination has four bytes by the precondition, and the two source
-    // pairs are disjoint from it because `DECIMAL_PAIRS` is static RO storage.
-    unsafe {
-        let pairs = DECIMAL_PAIRS.as_ptr();
-        let dst = buf.as_mut_ptr().cast_init();
+    unsafe { core::hint::assert_unchecked(high < 100 && low < 100) }
 
-        core::ptr::copy_nonoverlapping(pairs.add(high * 2), dst, 2);
-        core::ptr::copy_nonoverlapping(pairs.add(low * 2), dst.add(2), 2);
-    }
+    buf[0..2].write_copy_of_slice(&DECIMAL_PAIRS[high * 2..high * 2 + 2]);
+    buf[2..4].write_copy_of_slice(&DECIMAL_PAIRS[low * 2..low * 2 + 2]);
 }
 
 /// Encodes the 16 least-significant decimals of n into `buf[OFFSET .. OFFSET +
@@ -863,20 +855,20 @@ fn enc_16lsd<const OFFSET: usize>(buf: &mut [MaybeUninit<u8>], n: u64) {
         let quad = remain % 1_00_00;
         remain /= 1_00_00;
 
-        // SAFETY: `OFFSET + quad_index * 4` starts one of the four
-        // non-overlapping four-byte regions proven in bounds above.
-        unsafe {
-            write_quad(
-                buf.get_unchecked_mut(OFFSET + quad_index * 4..OFFSET + (quad_index + 1) * 4),
-                quad,
-            );
-        }
+        write_quad(
+            // SAFETY: `OFFSET + 16 <= buf.len()` and `quad_index < 4`, so this range is within `buf`.
+            unsafe {
+                buf.get_unchecked_mut(OFFSET + quad_index * 4..OFFSET + (quad_index + 1) * 4)
+            },
+            quad,
+        );
     }
 
-    // SAFETY: OFFSET starts the first four-byte region proven in bounds above.
-    unsafe {
-        write_quad(buf.get_unchecked_mut(OFFSET..OFFSET + 4), remain);
-    }
+    write_quad(
+        // SAFETY: `OFFSET + 16 <= buf.len()` was asserted above.
+        unsafe { buf.get_unchecked_mut(OFFSET..OFFSET + 4) },
+        remain,
+    );
 }
 
 /// Euclidean division plus remainder with constant 1E16 basically consumes 16

I applied this patch; it seems that now we can make write_quad safe and slightly reduce the amount of unsafe code in the places where it was previously needed. Also, to help the compiler, I had to add another assert_unchecked:

unsafe { core::hint::assert_unchecked(high < 100 && low < 100) }

for

    buf[0..2].write_copy_of_slice(&DECIMAL_PAIRS[high * 2..high * 2 + 2]);
    buf[2..4].write_copy_of_slice(&DECIMAL_PAIRS[low * 2..low * 2 + 2]);

[chirizxc]

GodBolt Diff

[clarfonthey]

@bors try
@rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 4127b8d (4127b8d7f5e00c0e865745f553479f23142d7771)
Base parent: e19d321 (e19d321c06479c6fd77533582b0d5a86651f1be3)

[rust-timer]

Finished benchmarking commit (4127b8d): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 1.4%, secondary -2.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.9%	[3.9%, 3.9%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	1.4%	[-1.1%, 3.9%]	2

Cycles

Results (primary 2.8%, secondary -2.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.8%	[2.8%, 2.8%]	1
Regressions ❌ (secondary)	3.6%	[2.6%, 4.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.9%	[-6.1%, -2.1%]	7
All ❌✅ (primary)	2.8%	[2.8%, 2.8%]	1

Binary size

Results (primary -0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.0%	[-0.0%, -0.0%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-0.0%, -0.0%]	2

Bootstrap: 490.165s -> 495.104s (1.01%)
Artifact size: 388.34 MiB -> 390.35 MiB (0.52%)

[clarfonthey]

@bors r+

Thank you! Don't think we need a second perf run if you've confirmed the new changes don't affect anything.

[rust-bors]

📌 Commit c257aef has been approved by clarfonthey

It is now in the queue for this repository.

[inkreasing]

I don't know if std has some special convention, but does this function have to become unsafe?
Since it now depends on the caller not passing n >= 10_000_000_000_000_000 otherwise UB happens.

View changes since the review

[clarfonthey]

Yes, this should be marked as unsafe. My bad; I thought both functions were remarked as unsafe, but I guess not.

[clarfonthey]

Trying to understand why I missed this, and I guess it's because GitHub has been doing a horrible job trying to show partial diffs when I review things instead of full diffs, and I thought I had reviewed the full diff, but I guess not.

.-.

[clarfonthey]

@bors r-

See: #159130 (comment)

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#160174), which was unapproved.

View changes since this unapproval

[rust-bors]

⚠️ A new commit b5e581ffb28d29a2382e461498b45221177bde82 was pushed.

This PR was contained in a rollup (#160174), which was closed.

[inkreasing]

I don't know if this is taking over review (sorry if it is), but if buf has to be exactly four elements long, why not do it like this?

Suggested change

	unsafe fn write_quad(buf: &mut [MaybeUninit<u8>], quad: u64) {
	unsafe fn write_quad(buf: &mut [MaybeUninit<u8>; 4], quad: u64) {

Using it should not be too hard, since https://doc.rust-lang.org/std/primitive.slice.html#impl-TryFrom%3C%26mut+%5BT%5D%3E-for-%26mut+%5BT;+N%5D and if perf sensitive unwrap_unchecked.

View changes since the review

[chirizxc]

core::hint::assert_unchecked(buf.len() == 4)

already provides the compiler with the same guarantees a &mut [MaybeUninit<u8>; 4] would, so switching types wouldn't remove any unsafe or improve codegen, every call site already builds this slice via get_unchecked_mut(range), itself unsafe