Optimize ToString implementation for integers

[GuillaumeGomez]

Part of #135543.

Follow-up of #133247 and #128204.

Rather than writing pretty bad benchers like I did last time, @workingjubilee: do you have a suggestion on how to check the impact on performance for this PR? Thanks in advance!

r? @workingjubilee

[rustbot]

Could not assign reviewer from: workingjubilee.
User(s) workingjubilee are either the PR author, already assigned, or on vacation. Please use r? to specify someone else to assign.

[rustbot]

r? @Mark-Simulacrum

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

[GuillaumeGomez]

I'm very confused by the CI errors. Did I unfold a very weird bug somehow? Like 0 != 0. 😮

[workingjubilee]

specialization strikes again, perhaps

[theemathas]

It seems to me that the tests are testing for a known miscompilation: #107975

[theemathas]

I'm guessing that this PR caused the compiler to be able to figure out that calling to_string on a number doesn't cause that number to change, allowing more optimizations to happen, causing the miscompilation to behave differently.

[workingjubilee]

lovely.

[GuillaumeGomez]

Fixed CI. So now about benching the change: any suggestion @workingjubilee ?

In the meantime:

@bors try @rust-timer queue

[bors]

⌛ Trying commit 83dc76e with merge 97c5b4b...

[bors]

☀️ Try build successful - checks-actions
Build commit: 97c5b4b (97c5b4b9bc9a34c7dde8738389ba12cb733cd54e)

[rust-timer]

Finished benchmarking commit (97c5b4b): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -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.4%	[-0.6%, -0.1%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.4%	[-0.6%, -0.1%]	2

Max RSS (memory usage)

Results (primary 0.3%, secondary 2.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)	3.2%	[3.2%, 3.2%]	1
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-2.6%	[-2.6%, -2.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[-2.6%, 3.2%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (primary 0.1%, secondary 0.2%)

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%, 1.0%]	22
Regressions ❌ (secondary)	0.5%	[0.5%, 0.5%]	3
Improvements ✅ (primary)	-0.1%	[-0.2%, -0.0%]	6
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	3
All ❌✅ (primary)	0.1%	[-0.2%, 1.0%]	28

Bootstrap: 776.287s -> 777.396s (0.14%)
Artifact size: 328.44 MiB -> 328.53 MiB (0.03%)

[workingjubilee]

I don't believe the tests should be changed in this way.

[GuillaumeGomez]

I'm not sure if we should keep the code and modify it (like I did) or just comment out the parts that are failing. Or maybe you see a third way?

[workingjubilee]

The point of these tests is the series of asserts, changing just one line or two that fails undermines the point.

[GuillaumeGomez]

The point was testing that calling to_string on them somehow changed the value. The new checks ensure that it doesn't and I commented as such. Not too sure what else to do on them.

[workingjubilee]

These are known-bug tests.

They are testing for the existence of a bug.

We did not fix the bug, because there are other ways to reach the compilation quirks in question. They have little to nothing to do with to_string in particular. We changed the code it was testing from under its feet, so the test has to be different code in order to still be a useful test, as what we want is to verify when LLVM stops miscompiling a sequence of LLVMIR.

[GuillaumeGomez]

Oh I see. So in short, calling any method on the type should still trigger the original bug iiuc. Let me give it a try. In the meantime, any idea on how to bench this PR?

[GuillaumeGomez]

You were right, just calling another method did the trick. Tests are now back to what they were.

[workingjubilee]

Thank you. I will think about that a bit.

[bors]

☔ The latest upstream changes (presumably #135265) made this pull request unmergeable. Please resolve the merge conflicts.

[GuillaumeGomez]

Fixed merge conflicts.

Also cc @tgross35 since you mentioned you were also working on this.

[Mark-Simulacrum]

r? @workingjubilee (I guess) -- happy to get re-rolled but seems like you might have more context here?

[rustbot]

Could not assign reviewer from: workingjubilee.
User(s) workingjubilee are either the PR author, already assigned, or on vacation. Please use r? to specify someone else to assign.

[tgross35]

Also cc @tgross35 since you mentioned you were also working on this.

If I am I'm not aware of it :)

[tgross35]

Oh, there was some confusion on another issue - I'm working on float string conversions, not integers.