Outline panicking code for RefCell::borrow and RefCell::borrow_mut

[Zoxc]

This outlines panicking code for RefCell::borrow and RefCell::borrow_mut to reduce code size.

[rustbot]

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

[saethlin]

Do you have a demonstration of this improving code size?

[Zoxc]

Here's an example function:

fn test(a: &RefCell<usize>) {
    *a.borrow_mut() = 0xF00D;
}

Before:

panic_test::test:
        sub rsp, 56

        cmp qword ptr [rcx], 0
        jne .LBB5_1

        mov qword ptr [rcx + 8], 61453
        mov qword ptr [rcx], 0

        add rsp, 56
        ret

.LBB5_1:
        lea rax, [rip + __unnamed_10]
        mov qword ptr [rsp + 32], rax
        lea rcx, [rip + __unnamed_11]
        lea r9, [rip + __unnamed_12]
        lea r8, [rsp + 55]
        mov edx, 16
        call core::result::unwrap_failed
        ud2

After:

panic_test::test:
        sub rsp, 40

        cmp qword ptr [rcx], 0
        jne .LBB6_2

        mov qword ptr [rcx + 8], 61453
        mov qword ptr [rcx], 0

        add rsp, 40
        ret

.LBB6_2:
        lea rcx, [rip + __unnamed_5]

        call core::cell::panic_already_borrowed
        ud2

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit 00c2511 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[klensy]

Now this will print on release "already borrowed: BorrowMutError" instead of "already borrowed", which is useless.
Next one similar.

[Zoxc]

That's done to match the existing output. BorrowMutError may also contain the location of the original borrower.

[klensy]

https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=4db78fa88be92de81e74933887a7a140
Oh, my bad, with this pr and previously that code outputs "already borrowed: BorrowMutError".

Sad to not to have test coverage. Can you add tests with panic outputs?

Can this be simple strings like "already borrowed" instead? If user wants more info from error he will use try_borrow/try_borrow_mut instead of borrow/borrow_mut, that fn should be as slim as possible (but that cold code anyway, except pulling debug impl for error). And location available only via debug_refcell which require rebuild std anyway?

[Zoxc]

It would make sense to me to use simpler strings with debug_refcell disabled.

[Mark-Simulacrum]

@bors r-

Good catch. r=me with those nits fixed

[Mark-Simulacrum]

@bors r+ since it sounds like this isn't a change in behavior

[bors]

📌 Commit 00c2511 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[bors]

⌛ Testing commit 00c2511 with merge abfc6c4...

[bors]

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing abfc6c4 to master...

[rust-timer]

Finished benchmarking commit (abfc6c4): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.0%	[0.6%, 1.5%]	8
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.3%	[-12.0%, -0.6%]	2
Improvements ✅ (secondary)	-0.5%	[-0.6%, -0.5%]	4
All ❌✅ (primary)	-0.4%	[-12.0%, 1.5%]	10

Max RSS (memory usage)

Results

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)	4.8%	[2.6%, 6.4%]	4
Regressions ❌ (secondary)	6.6%	[6.6%, 6.6%]	1
Improvements ✅ (primary)	-5.6%	[-5.6%, -5.6%]	1
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	1
All ❌✅ (primary)	2.7%	[-5.6%, 6.4%]	5

Cycles

Results

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)	4.6%	[3.8%, 5.4%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-4.4%	[-8.3%, -0.5%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.0%	[-8.3%, 5.4%]	5

Binary size

Results

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.9%	[0.1%, 1.7%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.4%, -0.0%]	40
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-0.4%, 1.7%]	42

Bootstrap: 628.382s -> 628.522s (0.02%)
Artifact size: 316.56 MiB -> 316.23 MiB (-0.10%)

[lqd]

bitmaps and cargo opt incr-unchanged kinda look noisy atm

[pnkfelix]

bitmaps and cargo opt incr-unchanged kinda look noisy atm

@rustbot label: +perf-regression-triaged