Simplify num formatting helpers

[yotamofek]

Noticed ilog10 was being open-coded when looking at this diff: https://github.com/rust-lang/rust/pull/143423/files/85d6768f4c437a0f3799234df20535ff65ee17c2..76d9775912ef3a7ee145053a5119538bf229d6e5#diff-6be9b44b52d946ccac652ddb7c98146a01b22ea0fc5737bc10db245a24796a45
That, and two other small cleanups 😁

(should probably go through perf just to make sure it doesn't regress formatting)

[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

[workingjubilee]

Thanks, let's run perf.

@bors2 try @rust-timer queue

[rust-bors]

⌛ Trying commit 09cf02c with merge 44935f5…

To cancel the try build, run the command @bors2 try cancel.

[tgross35]

Regardless of what perf winds up saying, I'm not sure about this; code size is about the same but you lose the early returns and add memory access https://rust.godbolt.org/z/sdejK8T9M. imo "flat" is the best of the three since the source is more readable and it has a smaller best-case icount than the other options.

[yotamofek]

Regardless of what perf winds up saying, I'm not sure about this; code size is about the same but you lose the early returns and add memory access https://rust.godbolt.org/z/sdejK8T9M. imo "flat" is the best of the three since the source is more readable and it has a smaller best-case icount than the other options.

Thanks for putting this into godbolt!
Well, the log variant has less branching, so that might be more performant, but 🤷
BTW, the "memory access" you mention, are you referring the leas? Because at a closer look, those are actually just the compiler's fancy way of doing addition, it's adding to rax those constants from here:

rust/library/core/src/num/int_log10.rs

Lines 31 to 34 in 8e61502

	const C1: u32 = 0b011_00000000000000000 - 10; // 393206
	const C2: u32 = 0b100_00000000000000000 - 100; // 524188
	const C3: u32 = 0b111_00000000000000000 - 1000; // 916504
	const C4: u32 = 0b100_00000000000000000 - 10000; // 514288

[hanna-kruppe]

Yeah, from that godbolt link the ilog10 variant looks obviously best to me. If it wasn't, the u16 ilog10 impl should probably be rewritten! All three functions have very similar code size, none access memory, so all the difference is in branches and data dependency structure. Singling out the best case icount seems misleading, since there's no reason to expect the Part::Num values to always have the same magnitude (let alone always be < 10, which is the best case of flat). If any of those branches are ever not predicted very well, the cost of misprediction will be significant relative to the amount of ALU work being done. And considering the context here (bunch of small, variable width memcpy's) I can't imagine having a larger pile of nice cheap ALU operations to chew through is going to bother the CPU.

FWIW, the AArch64 codegen has virtually no branches in flat and branches, instead compares + conditional selects/increments. So any of them is probably fine w.r.t. these aspects , but also no reason to not use ilog10.

[workingjubilee]

I am skeptical of more complicated and nuanced "simplifications" but usually ilog is the right choice.

[tgross35]

I stand very corrected here; I was thinking this would be more predictable, and read the brackets without looking closely (why does lea use that syntax...). No objection!

[rust-bors]

☀️ Try build successful (CI)
Build commit: 44935f5 (44935f5374c90b3c829aa2ee10fc1916f12ed5df, parent: de031bbcb161b0b7fc0eb16f77b02ce9fbdf4c9e)

[rust-timer]

Finished benchmarking commit (44935f5): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (primary 2.6%, secondary -5.1%)

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

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

Cycles

Results (primary -6.2%, secondary 6.3%)

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)	6.3%	[2.1%, 10.4%]	2
Improvements ✅ (primary)	-6.2%	[-6.2%, -6.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-6.2%	[-6.2%, -6.2%]	1

Binary size

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

Bootstrap: 460.528s -> 460.41s (-0.03%)
Artifact size: 372.13 MiB -> 372.12 MiB (-0.00%)

[tgross35]

@bors r+

[bors]

📌 Commit 09cf02c has been approved by tgross35

It is now in the queue for this repository.

[bors]

⌛ Testing commit 09cf02c with merge 040e2f8...

[bors]

☀️ Test successful - checks-actions
Approved by: tgross35
Pushing 040e2f8 to master...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 45b80ac (parent) -> 040e2f8 (this PR)

Test differences

Show 4 test diffs

4 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 040e2f8b9ff2d76fbe2146d6003e297ed4532088 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. dist-apple-various: 5136.2s -> 9552.8s (86.0%)
2. dist-aarch64-linux: 8049.2s -> 6011.9s (-25.3%)
3. dist-x86_64-apple: 11449.8s -> 8583.3s (-25.0%)
4. aarch64-apple: 4072.5s -> 4701.2s (15.4%)
5. x86_64-apple-2: 3510.0s -> 3058.6s (-12.9%)
6. x86_64-apple-1: 8012.8s -> 7022.5s (-12.4%)
7. dist-sparcv9-solaris: 4835.5s -> 5306.9s (9.7%)
8. dist-s390x-linux: 4718.7s -> 5116.6s (8.4%)
9. dist-arm-linux-musl: 5166.7s -> 5587.4s (8.1%)
10. dist-aarch64-apple: 5553.6s -> 5930.0s (6.8%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (040e2f8): comparison URL.

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

@rustbot label: -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%]	1
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-1.1%	[-1.1%, -1.1%]	1
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	1

Max RSS (memory usage)

Results (primary 3.6%)

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

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

Cycles

Results (primary 3.1%, secondary 4.0%)

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

	mean	range	count
Regressions ❌ (primary)	3.1%	[2.8%, 3.5%]	2
Regressions ❌ (secondary)	4.0%	[4.0%, 4.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.1%	[2.8%, 3.5%]	2

Binary size

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

Bootstrap: 467.247s -> 467.351s (0.02%)
Artifact size: 372.36 MiB -> 372.36 MiB (-0.00%)

[yotamofek]

The perf improvement is just noise, right? syn looks bifurcated