optimize char::to_digit and assert radix is at least 2

[programmerjake]

approved by t-libs: rust-lang/libs-team#475 (comment)

let me know if this needs an assembly test or similar.

[rustbot]

r? @thomcc

rustbot has assigned @thomcc.
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

[okaneco]

I wrote this comment before the last force push. The latest commit is branchless but from my testing in the past, that regresses the greater-than-10 radix case because the decimal fast path is removed. It'd be helpful to see the radix benchmark results to confirm this is an improvement.

https://rust.godbolt.org/z/9h14szYM7

---

Previously written comment
From my experience benchmarking this function, it's difficult to optimize this further than the current implementation. The current code generated tends to be the fastest for non-constant radix, decimal radix, and hexadecimal radix (or any greater-than-10 radix).

It looks like this will worsen performance for greater than decimal radices. Note the code path for converting 0-9 digits in example::to_digit_hex.
https://rust.godbolt.org/z/4a1947nsG - Current to_digit without the new assert conditions

[programmerjake]

fixed, and this should be decently optimized on x86_64:
https://rust.godbolt.org/z/MxW8MWa4Y
a function that calls to_digit in a loop over a slice of bytes to implement parsing
an integer (doesn't check for overflow) triggers LLVM's loop unswitching
to produce a separate loop for base <= 10 and base > 10:

base <= 10:

        ; slice pointer in rdi, slice len in rsi
        ; loop index in r8, radix in rcx
        xor     edx, edx ; retval = 0
.LBB0_5:
        movzx   r9d, byte ptr [rdi + r8]
        add     r9d, -48 ; value = byte - '0'
        cmp     r9, rcx  ; if value >= radix {
        jae     .LBB0_9  ; branch to error exit }
        imul    rdx, rcx ; retval *= radix
        add     rdx, r9  ; retval += value
        inc     r8
        cmp     rsi, r8
        jne     .LBB0_5

base > 10:

        ; slice pointer in rdi, slice len in rsi
        ; loop index in r8, radix in rcx
        xor     edx, edx        ; retval = 0
.LBB0_8:
        movzx   r10d, byte ptr [rdi + r8]
        mov     r11d, r10d
        or      r11d, 32        ; lower = byte | 0x20_u32
        add     r11d, -97       ; temp = lower - 'a' as u32
        add     r11, 10         ; letter_value = temp as u64 + 10
        lea     r9d, [r10 - 48] ; value = byte - '0' as u32
        cmp     r10d, 58        ; if byte > '9' as u32 {
        cmovae  r9, r11         ; value = letter_value }
        cmp     r9, rcx         ; if value >= radix {
        jae     .LBB0_9         ; branch to error exit }
        mov     r9d, r9d        ; value = value as u32 as u64 -- unnecessary but llvm can't optimize out
        imul    rdx, rcx        ; retval *= radix
        add     rdx, r9         ; retval += value
        inc     r8
        cmp     rsi, r8
        jne     .LBB0_8

[programmerjake]

here's what I got on my desktop (Ryzen 7950X):
master is on 116fc31
optimize-charto_digit is on aeffff8
Assuming I'm interpreting it right, my change is slightly faster on all to_digit benches except base 36 where it's slightly slower. I'm guessing the pattern used is simple enough that the branch predictor can learn it exactly, which is not realistic for normal hex numbers, thereby making the conditional move exactly as expensive as a conditional branch.
I ran:

for b in master optimize-charto_digit; do git switch "$b"; for i in {1..3}; do RUST_BACKTRACE=1 ./x.py bench library/core -- to_digit > "$b".txt; done; done

on optimize-charto_digit:

benchmarks:
    char::methods::bench_to_digit_radix_10  33672.73/iter +/- 127.29
    char::methods::bench_to_digit_radix_16  33518.58/iter  +/- 67.57
    char::methods::bench_to_digit_radix_2   33725.32/iter  +/- 92.59
    char::methods::bench_to_digit_radix_36  35903.44/iter +/- 126.43
    char::methods::bench_to_digit_radix_var 23992.17/iter  +/- 63.16

on master:

benchmarks:
    char::methods::bench_to_digit_radix_10  34082.78/iter  +/- 78.49
    char::methods::bench_to_digit_radix_16  34077.87/iter  +/- 61.22
    char::methods::bench_to_digit_radix_2   34084.60/iter  +/- 77.45
    char::methods::bench_to_digit_radix_36  34445.12/iter +/- 252.94
    char::methods::bench_to_digit_radix_var 25113.80/iter  +/- 36.69

[okaneco]

Nice, it does look faster except that one case.

For radix-36, the assembly is different in this part using your original loop example.
I wouldn't expect this difference to happen but I don't know enough about how the code is optimized.
https://rust.godbolt.org/z/qq4Y1erjG

; radix-36
        cmp     r8, 35
        ja      .LBB1_4
        lea     rdx, [rdx + 8*rdx]
        lea     rdx, [r8 + 4*rdx]

; radix-16
        cmp     r8, 15
        ja      .LBB1_4
        shl     rdx, 4
        or      rdx, r8

[rvolgers]

Nice, it does look faster except that one case.

For radix-36, the assembly is different in this part using your original loop example. I wouldn't expect this difference to happen but I don't know enough about how the code is optimized. https://rust.godbolt.org/z/qq4Y1erjG

This is normal optimizations of multiplication by a constant (the base, 36 vs 16).

; radix-36
        cmp     r8, 35
        ja      .LBB1_4
        lea     rdx, [rdx + 8*rdx]
        lea     rdx, [r8 + 4*rdx]

This means:

rdx = rdx + (rdx * 8)
rdx = r8 + (rdx * 4)

It uses the lea instruction to do multiplications by a factor of two and addition in a single instruction. It has factored multiplication by 36 into multiplication by 9 (as 8 + 1) and then by 4.

; radix-16
        cmp     r8, 15
        ja      .LBB1_4
        shl     rdx, 4
        or      rdx, r8

This means:

rdx = rdx * 16
rdx = rdx + r8

The first multiplication is done with a left shift because it's a power of two, and then addition is done with a bitwise or because it knows rdx and r8 never have the same bit set so carry is not an issue.

[joshtriplett]

@bors delegate+

@programmerjake Feel free to r+ when you're satisfied that it improves performance across the board.

[bors]

✌️ @programmerjake, you can now approve this pull request!

If @joshtriplett told you to "r=me" after making some further change, please make that change, then do @bors r=@joshtriplett

[joshtriplett]

Speaking for myself and not the rest of the libs team, I think "faster for every case except base 36" seems entirely reasonable, given the relative rarity of that case.

[programmerjake]

Speaking for myself and not the rest of the libs team, I think "faster for every case except base 36" seems entirely reasonable, given the relative rarity of that case.

so, since it's as fast as I can get it without spending a bunch of time I don't have, and isn't strictly an improvement due to the base 36 case, what should I do?

[joshtriplett]

@programmerjake I think we should merge it.

[programmerjake]

@bors r=@joshtriplett

[bors]

📌 Commit aeffff8 has been approved by joshtriplett

It is now in the queue for this repository.

[workingjubilee]

compiler has a number of callsites here, let's not have this confound examining a rollup's performance.

@bors rollup=never

[bors]

⌛ Testing commit aeffff8 with merge c82e0df...

[bors]

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing c82e0df to master...

[rust-timer]

Finished benchmarking commit (c82e0df): comparison URL.

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

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.2%, 0.4%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.6%	[-0.6%, -0.6%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.8%, secondary -1.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.6%	[1.6%, 1.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-4.2%	[-7.0%, -2.7%]	3
Improvements ✅ (secondary)	-1.8%	[-1.8%, -1.8%]	1
All ❌✅ (primary)	-2.8%	[-7.0%, 1.6%]	4

Cycles

Results (primary -0.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Binary size

Results (primary 0.0%, secondary -0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.1%]	6
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.0%	[-0.0%, -0.0%]	4
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.0%]	36
All ❌✅ (primary)	0.0%	[-0.0%, 0.1%]	10

Bootstrap: 787.308s -> 787.664s (0.05%)
Artifact size: 335.36 MiB -> 335.34 MiB (-0.01%)