[fereidani]

Hey, this is a follow-up to our PR #149784.

I want to:

1. Rename FillGapOnDrop of dedup_by to PanicGuard, IMHO this is a better name and matches the retain_mut.
2. I forgot to add #[inline(never)] for drop of panic guard of retain_mut in my original PR, I've added that.
3. I've done the same for drop of panic guard of dedup_by and I've added both #[cold] and #[inline(never)]

As drops are only triggered for panic case, It should not affect performance for non-panic common case.

I expect slight binary size reduction.

r? joboet

[rustbot]

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

[joboet]

1. Rename FillGapOnDrop of dedup_by to PanicGuard, IMHO this is a better name and matches the retain_mut.

While consistency is nice, descriptiveness is even better – as such I think FillGapOnDrop is the better name. And the shortening from gap to g isn't by any means significant enough to warrant the reduced legibility.

2. I forgot to add #[inline(never)] for drop of panic guard of retain_mut in my original PR, I've added that.

In most cases, #[cold] suffices. Do you have any concrete examples where this turns up?

3. I've done the same for drop of panic guard of dedup_by and I've added both #[cold] and #[inline(never)]

As it is only called in catch-pads, I don't know how relevant this is. For these kinds of things it's always a good idea to look at the assembly and see whether your intuition is correct.

[fereidani]

It seems it reduces retain function body size at least in this godbolt test: https://godbolt.org/z/vGbP9P6WT
I think it might help in general cases where multiple of this panic handler would have been inlined in the user code, I'll request a perf run to see if it is effective.

[Kobzol]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 0a61e4d (0a61e4dd1357c41f0deb96d302381aea3013290b)
Base parent: e457a7b (e457a7b0d326d67b4322ef0d11bd715cfaeda48f)

[rust-timer]

Finished benchmarking commit (0a61e4d): comparison URL.

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

Max RSS (memory usage)

Results (primary 2.8%, secondary -4.2%)

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

	mean	range	count
Regressions ❌ (primary)	2.8%	[2.8%, 2.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.2%	[-6.6%, -1.8%]	2
All ❌✅ (primary)	2.8%	[2.8%, 2.8%]	1

Cycles

Results (primary 0.3%, secondary 2.5%)

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

	mean	range	count
Regressions ❌ (primary)	2.7%	[2.3%, 3.0%]	2
Regressions ❌ (secondary)	2.5%	[2.5%, 2.5%]	1
Improvements ✅ (primary)	-2.0%	[-2.1%, -2.0%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[-2.1%, 3.0%]	4

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 476.343s -> 473.583s (-0.58%)
Artifact size: 402.74 MiB -> 402.81 MiB (0.02%)

[rust-bors]

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

[joboet]

@rustbot reroll
I don't feel too strongly about this, and won't be able to respond in a timely manner...

[jhpratt]

I think FillGapOnDrop is much clearer as to intent than PanicGuard, so I'd prefer that name be kept. 👍 on #[inline(never)] and #[cold], though.

This also needs a rebase…maybe not if it's slimmed down.

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.