Display of integers without raw pointers and without overflowing_literals

[pascaldekloe]

The benchmarks as is measure formatting speed of literals. The first commit black_box-es input to simulate runtime speed instead.

The second commit replaces unsafe pointer optimizations with plain array indices. The performance is equivalent on Apple M1. Needs peer review on Intel.

Happy to do the 128-bit version too if such change is welcome.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Noratrieb (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[tgross35]

r? tgross35 since I'm doing some work in this area anyway

You should cd src/etc/test-float-parse and run cargo +stage0 run --release, those tests are technically for parsing but do invoke the print routines quite a bit. I'll take a closer look at the changes soon.

[pascaldekloe]

No problem if this one gets rejected in favour of anything else @tgross35. I was just pleased to see the compiler doing this well.

[tgross35]

You should cd src/etc/test-float-parse and run cargo +stage0 run --release, those tests are technically for parsing but do invoke the print routines quite a bit. I'll take a closer look at the changes soon.

Actually ignore this, I was thinking this was in flt2dec and not integers.

No problem if this one gets rejected in favour of anything else @tgross35. I was just pleased to see the compiler doing this well.

No conflict :) Replacing unsafe code is always nice if there are no drawbacks.

[tgross35]

It looks like there may be some small regressions here, the new code doesn't elide a couple calls to panic_bounds_check https://rust.godbolt.org/z/hjqhqsnbb. You could probably play around on godbolt a bit to figure out where this is coming from, a call to hint::assert_unchecked in the right place would likely make this disappear.

Soon it will be possible to write_copy_of_slice on the MaybeUninit slices, I don't expect this to do anything for optimization but it would allow keeping the "write two bytes at a time" feature from the original #129259.

[pascaldekloe]

What a gem that Compiler Explorer is @tgross35. I think the assembly looks good now.

B.t.w., I tried bit shifting entries from the lookup table into a 64-bit register and write per 8 digits at once. The compiler "optimizes" such intermediate register away, and it goes for 16-bit copies instead. Maybe those small writes are actually faster. I don't know. 😂

[pascaldekloe]

I found a way without compiler hinting which is even faster than the unsafe code as is. It requires buffers of an even byte-size. Needs a little more work for all integer sizes.

https://github.com/pascaldekloe/b10/blob/e7d9813c1f74dc9137216f1b271303fb056e90ad/src/lib.rs#L1723

[tgross35]

What a gem that Compiler Explorer is @tgross35.

I am by no means a perf expert but I still can't imagine chasing down optimizations without it :)

I found a way without compiler hinting which is even faster than the unsafe code as is. It requires buffers of an even byte-size. Needs a little more work for all integer sizes.

https://github.com/pascaldekloe/b10/blob/e7d9813c1f74dc9137216f1b271303fb056e90ad/src/lib.rs#L1723

Are you planning to update the PR with something like this? At a surface level the patch seems pretty reasonable but I didn't look too deep yet, I'll hold off on reviewing if you have something faster coming.

[pascaldekloe]

Yes, will give it a try @tgross35. Same surprise here as you're having. 🙂

[tgross35]

@rustbot author

(just comment @rustbot review to update the labels when this is ready for a look)

[pascaldekloe]

The speed gain is because (1) the loop decision no longer depends on the digit calculation, (2) it produces fewer instructions, and (3) it no longer needs a check for special case zero, which has a "significant" leading zero.

Unfortunately the results are less optimal for the two benchmark cases in the core library: either zero or maximum digits in a loop. Apparently the branch prediction benefit does not outweight the per-4-digit tweak in such scenario.

So now we have the explicit assertions as hint assertions too. Can you have a look @tgross35? @rustbot review

https://rust.godbolt.org/z/h97MacKcc

[bors]

⌛ Testing commit 821bc70 with merge 60f48c4...

[bors]

💔 Test failed - checks-actions

[tgross35]

@bors r=tgross35,ChrisDenton

[bors]

📌 Commit ebeaf2e has been approved by tgross35,ChrisDenton

It is now in the queue for this repository.

[bors]

⌛ Testing commit ebeaf2e with merge 019fc4d...

[bors]

☀️ Test successful - checks-actions
Approved by: tgross35,ChrisDenton
Pushing 019fc4d to master...

[rust-timer]

Finished benchmarking commit (019fc4d): comparison URL.

Overall result: ✅ 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
Improvements ✅ (primary)	-0.5%	[-0.6%, -0.5%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-0.6%, -0.5%]	2

Max RSS (memory usage)

Results (secondary 2.7%)

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)	2.7%	[2.7%, 2.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (primary 1.0%)

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.0%	[1.0%, 1.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.0%	[1.0%, 1.0%]	1

Binary size

Results (primary -0.2%, 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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.4%, -0.0%]	12
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	38
All ❌✅ (primary)	-0.2%	[-0.4%, -0.0%]	12

Bootstrap: 778.131s -> 778.858s (0.09%)
Artifact size: 328.78 MiB -> 328.82 MiB (0.01%)