Mark <mem::DropGuard as Drop>::drop as #[inline]

[GrigorenkoPV]

View all comments

#161702 has caused a perf regression. As suggested in #161702 (review), this might help.

This is a revert of #162128 (which was a revert of #161702) + <mem::DropGuard as Drop>::drop is now marked #[inline].

Tracking issue: #144426

[rust-bors]

@GrigorenkoPV: 🔑 Insufficient privileges: not in try users

[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, joboet, nia-e

[lqd]

@bors try @rust-timer queue

[clarfonthey]

r? clarfonthey since I reviewed the other change

[rust-bors]

☀️ Try build successful (CI)
Build commit: 4606595 (4606595922d66540e5fa94815f9717898f3ffc1d)
Base parent: 45f215f (45f215f136e00d8a74c69afde2f71be3f16837cf)

[rust-timer]

Finished benchmarking commit (4606595): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@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.9%	[0.3%, 1.5%]	2
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-0.4%	[-0.7%, -0.2%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.7%, 1.5%]	7

Max RSS (memory usage)

Results (primary 2.6%, secondary -2.9%)

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

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

Cycles

Results (secondary -3.2%)

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)	2.2%	[2.0%, 2.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.4%	[-7.1%, -2.4%]	9
All ❌✅ (primary)	-	-	0

Binary size

Results (primary -0.5%, secondary -1.1%)

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

	mean	range	count
Regressions ❌ (primary)	1.3%	[1.3%, 1.3%]	1
Regressions ❌ (secondary)	0.7%	[0.7%, 0.7%]	1
Improvements ✅ (primary)	-0.6%	[-11.1%, -0.0%]	49
Improvements ✅ (secondary)	-1.3%	[-11.1%, -0.0%]	11
All ❌✅ (primary)	-0.5%	[-11.1%, 1.3%]	50

Bootstrap: 475.395s -> 473.906s (-0.31%)
Artifact size: 401.38 MiB -> 400.48 MiB (-0.22%)

[clarfonthey]

Tempted to say these are a net positive but will take another look later.

[panstromek]

This is positive, yes, especially if you look at binary sizes - in some benchmarks, like html5ever-0.31.0 it more then offsets the regression from the original PR.

This doesn't address the original regression so we have to be a bit careful to asses the impact overall. It's possible (but unlikely in my opinion) that this is only positive because #161702 was negative and the benefit of this PR largely disappears when we address the root cause. Not a blocking concern, but something to keep in mind for example if later results come back unexpectedly red.

[clarfonthey]

Right, this does not address the main regression, but still is a net positive, IMO. I guess I'll hold off on merging in case we'd rather revert the original before implementing a proper fix.

[clarfonthey]

Side note: with the full revert in #162128, it's up to you if you want to reuse this PR or open a new one for whatever fix you come up with. I'm not sure I can delegate perf runs, but will try and run them when you ask.

[clarfonthey]

@rustbot author

[rustbot]

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

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[GrigorenkoPV]

I've rebased this on top of the full revert in #162128. And reverted the revert. Let's maybe see what the perf implications of #[inline] + #161702 are.

[panstromek]

Cool. Looks like bors didn't pick up the edit?

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 4c21a3f (4c21a3f4f1966d672b24c44614c7d78ddc18e069)
Base parent: c25253f (c25253fac451fff60fb5b6c0b2c64d60f628c7cd)

[rust-timer]

Finished benchmarking commit (4c21a3f): comparison URL.

Overall result: ❌ regressions - BENCHMARK(S) FAILED

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

❗ ❗ ❗ ❗ ❗
Warning ⚠️: The following benchmark(s) failed to build:

• Job failure

❗ ❗ ❗ ❗ ❗

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.5%	[0.2%, 2.0%]	44
Regressions ❌ (secondary)	0.6%	[0.2%, 1.8%]	25
Improvements ✅ (primary)	-0.5%	[-0.7%, -0.3%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[-0.7%, 2.0%]	48

Max RSS (memory usage)

Results (primary -0.1%, 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)	4.0%	[4.0%, 4.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.2%	[-2.3%, -2.1%]	2
Improvements ✅ (secondary)	-2.5%	[-3.6%, -1.6%]	3
All ❌✅ (primary)	-0.1%	[-2.3%, 4.0%]	3

Cycles

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

Binary size

Results (primary 1.3%, secondary 1.3%)

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

	mean	range	count
Regressions ❌ (primary)	1.4%	[0.3%, 2.6%]	62
Regressions ❌ (secondary)	1.3%	[0.4%, 2.9%]	30
Improvements ✅ (primary)	-0.5%	[-0.6%, -0.4%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.3%	[-0.6%, 2.6%]	64

Bootstrap: missing data
Artifact size: 400.67 MiB -> 400.42 MiB (-0.06%)

[panstromek]

The failure looks unrelated. cc @Kobzol @JonathanBrouwer is this possibly related to something you're working on right now?

[Kobzol]

No, it looks like some (hopefully transient) error in fetching SHAs from GitHub on the collector machines 🤔 It should only affect the bootstrap build.