Arc: Remove unnecessary fmt::Display use for overflow assertion

[fereidani]

• I did not use an LLM to create a change in this PR.
• I used an LLM to create a change in this PR, and I have explained below how it was used.

This PR removes unnecessary fmt::Display generated assembly because of use of formatting in the overflow check assertion(assert!(n <= MAX_REFCOUNT, "{}", INTERNAL_OVERFLOW_ERROR);).

This reduces generated x64 assembly for counter increment(simplified) from:

increase_counter:
        test    rdi, rdi
        js      .LBB1_2
        inc     rdi
        mov     rax, rdi
        ret
.LBB1_2:
        sub     rsp, 24
        lea     rax, [rip + .Lanon.642d46026bcfa00bfed8baf8cdbf502d.2]
        mov     qword ptr [rsp + 8], rax
        lea     rax, [rip + <&str as core[4e7ceada952a6ea0]::fmt::Display>::fmt]
        mov     qword ptr [rsp + 16], rax
        lea     rdi, [rip + .Lanon.642d46026bcfa00bfed8baf8cdbf502d.0]
        lea     rdx, [rip + .Lanon.642d46026bcfa00bfed8baf8cdbf502d.4]
        lea     rsi, [rsp + 8]
        call    qword ptr [rip + core[4e7ceada952a6ea0]::panicking::panic_fmt@GOTPCREL]

.Lanon.642d46026bcfa00bfed8baf8cdbf502d.0:
        .asciz  "\300"

.Lanon.642d46026bcfa00bfed8baf8cdbf502d.1:
        .ascii  "Arc counter overflow"

To:

increase_counter:
        test    rdi, rdi
        js      .LBB0_2
        inc     rdi
        mov     rax, rdi
        ret
.LBB0_2:
        push    rax
        lea     rdi, [rip + .Lanon.642d46026bcfa00bfed8baf8cdbf502d.0]
        lea     rdx, [rip + .Lanon.642d46026bcfa00bfed8baf8cdbf502d.2]
        mov     esi, 41
        call    qword ptr [rip + core[4e7ceada952a6ea0]::panicking::panic_fmt@GOTPCREL]

.Lanon.642d46026bcfa00bfed8baf8cdbf502d.0:
        .ascii  "Arc counter overflow"

Godbolt link:
https://godbolt.org/z/3qjxooYWM

I expect bors to report a binary size reduction for all programs using Arc. There might also be performance improvements as a byproduct of CPU cache hit increase.

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
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 JohnTitor, Mark-Simulacrum, clarfonthey, nia-e

[panstromek]

@bors try @rust-timer queue

[panstromek]

Note that I think we probably don't excercise this codepath very much in the compiler, so we probably won't see much impact in rustc-perf benchmarks. Are you trying to optimize something specific in the compiler or what's the reason you want to run perf on this?

[fereidani]

Thank you for your clarification, Currently one of my microbenchmarked PRs is getting rollback because of bors bench reporting regression after the merge, So I assumed every performance related change requires a bors report.
Please let me know if there is any guideline for requesting bors perf run.
Please feel free to cancel the run if you think it is unnecessary.

[rust-bors]

☀️ Try build successful (CI)
Build commit: babfadb (babfadb5bf31a7a10400372f0a8470e642b35d40)
Base parent: 7dfb9d0 (7dfb9d0c40d3338e1a27f0f787182c6b90e6791e)

[panstromek]

No worries, it's totally fine to do a perf run when you're not sure about the potential perf impact here.

So I assumed every performance related change requires a bors report.

rustc-perf measures mostly compiler performance, so it makes sense only when the change is expected to have an impact on that. Sometimes that's not easy to tell, so it's fine to run benchmarks just in case.

---

For perf changes that don't impact compiler performance, it's a bit less straightforward. There are some runtime benchmarks on rustc-perf (in "runtime benchmarks" tab in the report), but very few and they don't cover that many cases.

Apart from that, we often have codegen tests for these, or people asses the performance impact locally and include the benchmarks in the repo (we don't run those on CI, though, that would be too noisy).

The specific procedure depends on the change. This PR is fairly straightforward and it's a cleanup on its own so I don't think it'll need much more convincing. But It'd be nice to capture this in a codegen test since you've already looked at the assembly and posted it in the description.

[rust-timer]

Finished benchmarking commit (babfadb): 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.4%	[0.4%, 0.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.4%, secondary -0.1%)

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.6%	[0.4%, 1.0%]	9
Improvements ✅ (primary)	-2.4%	[-2.4%, -2.4%]	1
Improvements ✅ (secondary)	-2.2%	[-3.8%, -0.5%]	3
All ❌✅ (primary)	-2.4%	[-2.4%, -2.4%]	1

Cycles

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.9%	[0.5%, 2.3%]	10
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.9%	[-1.8%, -0.4%]	10
All ❌✅ (primary)	-	-	0

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

Bootstrap: 458.758s -> 459.208s (0.10%)
Artifact size: 398.65 MiB -> 398.53 MiB (-0.03%)

[joboet]

I think you could do even better by outlining the panicking case into a separate, #[cold] function. See e.g. the unwrap_failed function called in the None case of Option::unwrap for an example of this pattern.

[fereidani]

@panstromek Thank you for your complete and detailed explanation, I will look into the codegen tests.

@joboet Thanks alot for the hint! It removes 3 more instructions! just a push rax and call to the function.

increase_counter:
        test    rdi, rdi
        js      .LBB1_2
        inc     rdi
        mov     rax, rdi
        ret
.LBB1_2:
        push    rax
        call    example[4823d063a3151a8b]::panic_arc_overflow

[fereidani]

Is there anyway to rerun the CI? It seem unrelated to my changes.

r? joboet

joboet is currently at their maximum review capacity.
They may take a while to respond.

Is there anyway to rerun the CI? It seem unrelated to my changes.

Try force-pushing with a new commit hash:

git commit --amend --no-edit && git push --force

Thanks!
@bors r+

📌 Commit c1bfb4b has been approved by joboet

It is now in the queue for this repository.