Add #[inline] annotation to functions moved out of std::io

[bushrat011899]

View all comments

Description

• Attempts to resolve compilation time regressions noted here.

Solution

Added #[inline] to public functions in alloc::io and core::io with total disregard to rhyme or reason. Might make things better, might make them worse, let's find out!

---

Notes

• No AI tooling of any kind was used during the creation of this bodge PR.

[rustbot]

r? @clarfonthey

rustbot has assigned @clarfonthey.
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 6 candidates

[bushrat011899]

@JonathanBrouwer and/or @clarfonthey could we try a perf-run on this PR? I'm curious if this makes things better or worse for the core::io migration regressions.

[clarfonthey]

@bors try @rust-timer queue

Side note: very amused that this randomly chose me as the reviewer even though you didn't specify manually.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 71248b2 (71248b2ef0eb27d87ea455b1bdc956e55f92e04c)
Base parent: 38a0576 (38a0576951fb5f7209c892b030f40ac166ae86b4)

[rust-timer]

Finished benchmarking commit (71248b2): 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)	1.2%	[1.0%, 1.4%]	2
Regressions ❌ (secondary)	0.2%	[0.2%, 0.3%]	2
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[-1.0%, 1.4%]	3

Max RSS (memory usage)

Results (primary -2.2%, secondary 4.7%)

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

	mean	range	count
Regressions ❌ (primary)	0.9%	[0.9%, 0.9%]	1
Regressions ❌ (secondary)	4.7%	[4.2%, 5.2%]	2
Improvements ✅ (primary)	-3.3%	[-3.9%, -2.2%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.2%	[-3.9%, 0.9%]	4

Cycles

Results (primary 2.8%, secondary -1.9%)

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)	2.7%	[2.7%, 2.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.4%	[-3.5%, -3.2%]	3
All ❌✅ (primary)	2.8%	[2.8%, 2.8%]	1

Binary size

Results (primary 0.2%, secondary 0.2%)

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

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.0%, 1.6%]	6
Regressions ❌ (secondary)	0.2%	[0.0%, 0.3%]	2
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[-0.5%, 1.6%]	7

Bootstrap: 489.951s -> 488.986s (-0.20%)
Artifact size: 389.41 MiB -> 390.00 MiB (0.15%)

[bushrat011899]

Ok so the most notable result I can see is the improvement to cargo, which I believe nullifies (most of) the negatives that came from moving Write. I'll need to double check, but I think ripgrep's regression here is actually the inverse of an improvement from the Error move.

Either way, I think it's clear that some of these functions probably shouldn't be inline. Now to work out which 🙃

EDIT:

Actually never mind ripgrep has just gotten worse with every migration. This PR reverts the worst regressions from Write, but adds doesn't help Error.

[bushrat011899]

@clarfonthey Ok I've looked back over the core::io::Write commit, and I've restricted the addition of inline attributes to the handful of functions that I think caused the cargo compile time regression. If you're able to request another perf run that'd be awesome. I'm hoping the results of this run should be an improvement to cargo-0.87.1 by approximately 1%, with no other effects.

If this doesn't work, then I think I'll need assistance from someone more experienced in rustc performance tuning to assist.

[clarfonthey]

@bors try @rust-timer queue

side note: would offer to delegate permissions for this but not sure if that's possible

[rust-bors]

❌ Encountered an error while executing command

[clarfonthey]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: fcef807 (fcef807988686486ec5df900b495473895e92351)
Base parent: 3d50c25 (3d50c25bc66853bf0ad205529d0f305a1d841b5e)

[rust-timer]

Error occured while categorizing benchmark run:

could not find start commit for bound Commit("3d50c25bc66853bf0ad205529d0f305a1d841b5e")

[bushrat011899]

Looks like the base commit doesn't have perf data yet, but doing a comparison the next oldest as a base here shows the same ripgrep and cargo primary improvement/regressions as before.

Therefore, I think it's a wash whether to actually merge this PR; it improves cargo by 1% and regresses ripgrep by about the same. Personally, I think this is worth closing and continuing on with the various IO PRs I have open. I think the compilation performance difference is something a little more nuanced that I think I have the capability to resolve.

[clarfonthey]

I'm fine with that assessment.