Move std::io::Read to alloc::io

[bushrat011899]

View all comments

ACP: rust-lang/libs-team#755
Tracking issue: #154046
Split From: #156527
Blocked On: #158541

Description

Moves std::io::Read to alloc::io. This is mostly a direct cut and paste, but with a large diff due to how many implementations need to be moved into alloc. Blocked on #158541.

There is some new code written for some of the Read implementations to account for the no_global_oom_handling configuration, since Vec::extend_from_slice and String::push_str are unavailable. I've highlighted all of these such instances with review comments for clarity.

---

Notes

• No AI tooling of any kind was used during the creation of this PR.
• Please see Tracking Issue for alloc::io and core::io #154046 (comment) for a review order and broader context for this PR.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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: @ChrisDenton, libs
• @ChrisDenton, libs expanded to 13 candidates
• Random selection from 6 candidates

[bushrat011899]

@rustbot blocked

[bushrat011899]

@rustbot ready
From this point on, std::io items can only be moved to alloc, as Read depends on Vec and String, and everything left either directly or indirectly relies on Read.

[bushrat011899]

@rustbot label -T-compiler -T-rustdoc
I am pretty sure these labels are incorrectly applied because of some earlier rebase. T-rustdoc-frontend I believe is appropriate since the hardcoded URLs in one of their tests is now different.

[clarfonthey]

Doesn't actually matter, but I guess this should be alloc_io_internals since Read can't be moved to core yet.

View changes since the review

[bushrat011899]

Yeah I think I'll leave this one as-is if that's ok? Just makes things a tiny bit cleaner, even if the label isn't completely accurate.

[bushrat011899]

Based on this comment from the core::io::Write PR, it might be worth doing a rust-timer build to see if there's a similar performance regression in compile times? Both this and that PR are very similar in effect, so if there's a regression in one I'd expect it here too.

[clarfonthey]

I haven't fully reviewed this, but I'm going to mark this as blocked on figuring out the write regressions first. Since these all depend on each other, we probably want to figure out what to do there before continuing.

@rustbot blocked

[JonathanBrouwer]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 234ef2c (234ef2c008a7112fbea2e9e8553bcb4c11f7e135)
Base parent: da80ed0 (da80ed0708a09dc096c184345d6eb42cbcd50a1e)

[rust-timer]

Finished benchmarking commit (234ef2c): comparison URL.

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

Max RSS (memory usage)

Results (primary 1.0%, secondary -4.8%)

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

	mean	range	count
Regressions ❌ (primary)	4.5%	[4.5%, 4.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-4.8%	[-7.4%, -2.1%]	2
All ❌✅ (primary)	1.0%	[-2.5%, 4.5%]	2

Cycles

Results (secondary -1.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.9%	[2.5%, 3.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-4.0%, -2.1%]	4
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 491.202s -> 490.122s (-0.22%)
Artifact size: 389.32 MiB -> 389.39 MiB (0.02%)

[JonathanBrouwer]

(The regressions are noise)

[bushrat011899]

I'm surprised this had no real regressions. Pleasantly surprised, but still. I'll draft up a PR adding #[inline] to all the functions that were in std but are now accessed from core/alloc.

[clarfonthey]

Yeah, my best guess is that the regressions might be hiding in the code in alloc that depends on code in core, which is why the previous PR regressed and this one didn't.