Fix excessive initialization and reads beyond EOF in `io::copy(_, Vec<u8>)` specialization #117576

the8472 · 2023-11-04T15:02:33Z

rustbot · 2023-11-04T15:02:41Z

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

It now keeps track of initialized bytes to avoid reinitialization. It also keeps track of read sizes to avoid initializing more bytes than the reader needs. This is important when passing a huge vector to a Read that only has a few bytes to offer and doesn't implement read_buf().

The initial probe-for-empty-source by stack_buffer_copy only detected EOF if the source was empty but not when it was merely small which lead to additional calls to read() after Ok(0) had already been returned in the stack copy routine

Mark-Simulacrum · 2023-11-04T16:31:11Z

@bors r+

I wonder if the detection here should be publicly available on BorrowedBuf or similar - seems like it might be useful elsewhere too. But shouldn't block this.

bors · 2023-11-04T16:31:14Z

📌 Commit 78aa5e5 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

bors · 2023-11-06T00:06:00Z

⌛ Testing commit 78aa5e5 with merge 7a892ab...

bors · 2023-11-06T02:06:27Z

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

rust-timer · 2023-11-06T17:29:18Z

Finished benchmarking commit (7a892ab): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

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

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)	1.0%	[0.7%, 1.4%]	2
Regressions ❌ (secondary)	2.5%	[2.5%, 2.5%]	1
Improvements ✅ (primary)	-0.6%	[-0.8%, -0.4%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.8%, 1.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)	0.6%	[0.5%, 0.7%]	3
Regressions ❌ (secondary)	0.4%	[0.4%, 0.4%]	1
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	1
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	1
All ❌✅ (primary)	0.3%	[-0.5%, 0.7%]	4

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 636.185s -> 637.484s (0.20%)
Artifact size: 304.50 MiB -> 304.49 MiB (-0.00%)

a1phyr · 2023-11-09T08:59:29Z

Side question: is there a reason why this is not implemented as reader.read_to_end(self) ?

the8472 · 2023-11-09T09:12:10Z

Good point, they're quite similar though they contain some different optimizations. It probably make sense to unify them.

the8472 · 2023-11-09T09:13:32Z

beta-nominated on behalf of this comment

…Simulacrum [stable] Prepare 1.74.0 release https://forge.rust-lang.org/release/process.html#promote-branches-t-3-days-monday Also backports: * Disabling specialization as an alternative backport of "Fix excessive initialization and reads beyond EOF in io::copy(_, Vec<u8>) specialization rust-lang#117576" * coverage: Avoid creating malformed macro name spans rust-lang#117827 r? `@Mark-Simulacrum`

cuviper · 2023-11-17T00:49:16Z

This was backported during the 1.74.0 stable promotion in #117843, and it already landed in what is now 1.75-beta, so there's nothing more to do here.

@rustbot label -beta-nominated

unify read_to_end and io::copy impls for reading into a Vec This ports over the initial probe (to avoid allocation) and the dynamic read sizing from the io::copy specialization to the `default_read_to_end` implementation which already had its own optimizations for different cases. I think it should be a best-of-both now. suggested by `@a1phyr` in rust-lang#117576 (comment)

Rollup merge of rust-lang#118222 - the8472:copy-use-vec-write, r=m-ou-se unify read_to_end and io::copy impls for reading into a Vec This ports over the initial probe (to avoid allocation) and the dynamic read sizing from the io::copy specialization to the `default_read_to_end` implementation which already had its own optimizations for different cases. I think it should be a best-of-both now. suggested by `@a1phyr` in rust-lang#117576 (comment)

unify read_to_end and io::copy impls for reading into a Vec This ports over the initial probe (to avoid allocation) and the dynamic read sizing from the io::copy specialization to the `default_read_to_end` implementation which already had its own optimizations for different cases. I think it should be a best-of-both now. suggested by `@a1phyr` in rust-lang/rust#117576 (comment)

Update Rust toolchain from nightly-2023-11-06 to nightly-2023-11-07 without any other source changes. This is an automatically generated pull request. If any of the CI checks fail, manual intervention is required. In such a case, review the changes at https://github.com/rust-lang/rust from rust-lang@fee5518 up to rust-lang@189d6c7. The log for this commit range is: rust-lang@189d6c71f3 Auto merge of rust-lang#117641 - matthiaskrgr:rollup-f9c12td, r=matthiaskrgr rust-lang@9efe60b1eb Rollup merge of rust-lang#117637 - lqd:trivial-bounds-with-binder-vars, r=compiler-errors rust-lang@2a1f8bccee Rollup merge of rust-lang#117631 - smarnach:error-request-doc-fix, r=ChrisDenton rust-lang@8a34fea835 Rollup merge of rust-lang#117516 - matthiaskrgr:test_effects_113375_oob, r=fee1-dead rust-lang@1683df7d3b Rollup merge of rust-lang#117190 - matthiaskrgr:test_effects_113381, r=fee1-dead rust-lang@fb61292105 Auto merge of rust-lang#117292 - estebank:issue-80446, r=davidtwco rust-lang@4b7aacaa4f Silence redundant error on typo resulting on binop rust-lang@aea82b268a Auto merge of rust-lang#117603 - HKalbasi:make-feature-additive, r=Nilstrieb rust-lang@58351ae03f add test for trivial bound not holding in `soa-derive` rust-lang@2beca157c9 check binders with bound vars for global bounds that don't hold rust-lang@b049093560 Auto merge of rust-lang#116988 - RalfJung:null, r=WaffleLapkin rust-lang@e1fcecb1b9 Auto merge of rust-lang#117630 - matthiaskrgr:rollup-v0d5p3f, r=matthiaskrgr rust-lang@3a096e96fa Documentation cleanup for core::error::Request. rust-lang@b88d62e222 Rollup merge of rust-lang#117615 - bjorn3:misc_changes, r=davidtwco rust-lang@ec35c03143 Rollup merge of rust-lang#117613 - jackh726:vacation, r=compiler-errors rust-lang@60dca87cff Rollup merge of rust-lang#117592 - thomcc:env-span-wrong, r=davidtwco rust-lang@6bf2fb3679 Auto merge of rust-lang#117578 - compiler-errors:derive-encode-in-rustc_type_ir, r=davidtwco rust-lang@f9b644636f Auto merge of rust-lang#117435 - SparrowLii:nightly_parallel, r=oli-obk,davidtwco rust-lang@fcca978aa0 Auto merge of rust-lang#117607 - klensy:clang-17.0.4, r=Mark-Simulacrum rust-lang@152a4e90d1 Auto merge of rust-lang#117585 - dnbln:feat/move-kw-span, r=cjgillot rust-lang@f2a40e99ff use portable AtomicU64 for powerPC and MIPS rust-lang@7a892ab8d8 Auto merge of rust-lang#117576 - the8472:fix-io-copy-vec, r=Mark-Simulacrum rust-lang@c8a25eddfe Make the randomize feature of rustc_abi additive rust-lang@ec29a02071 Update doc comment for CodegenBackend::link rust-lang@ba82056a14 Use the actual computed crate name for -Zprint-vtable-sizes rust-lang@1a1b10fa63 Don't steal the parse query when using --pretty rust-lang@6e33e89337 Remove from vacation and compiler review group rust-lang@38ff91c453 bump clang version for dist-x86_64-linux from 17.0.2 to 17.0.4 rust-lang@86fca873ba Use the correct span when emitting the `env!` result rust-lang@c077147200 fix clippy author and failing test rust-lang@876f698790 Add the vis.visit_capture_by() in noop_visit_expr rust-lang@54ce0346c0 add `fn visit_capture_by` to MutVisitor and fix pprust-expr-roundtrip.rs rust-lang@df85b28b72 fixes for rustfmt + ast visitor rust-lang@a6b41aa6ba fmt rust-lang@241a654c07 Fix remaining uses of `CaptureBy::Value` rust-lang@8de489918b feat(hir): Store the `Span` of the `move` keyword rust-lang@bee7b5889e Derive TyEncodable/TyDecodable implementations that are parameterized over interner rust-lang@78aa5e511c detect EOF earlier rust-lang@8d8f06b277 avoid excessive initialization when copying to a Vec rust-lang@666cad9a5f add test for rust-lang#113375 rust-lang@384f7382ba add test for rust-lang#113381 rust-lang@248dd14fa5 update config.example.toml rust-lang@bf5fb7614b update bootstrap change history rust-lang@ab8101d019 enable parallel rustc in nightly builds rust-lang@231f93524d Detect misparsed binop caused by missing semi rust-lang@98d54da1ee document that the null pointer has the 0 address --------- Co-authored-by: celinval <[email protected]> Co-authored-by: Zyad Hassan <[email protected]> Co-authored-by: Zyad Hassan <[email protected]>

rustbot assigned Mark-Simulacrum Nov 4, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 4, 2023

the8472 mentioned this pull request Nov 4, 2023

Add new test exposing mysterious bug bczhc/bzip3-rs#8

Merged

This comment has been minimized.

Sign in to view

the8472 added 2 commits November 4, 2023 16:11

detect EOF earlier

78aa5e5

The initial probe-for-empty-source by stack_buffer_copy only detected EOF if the source was empty but not when it was merely small which lead to additional calls to read() after Ok(0) had already been returned in the stack copy routine

the8472 force-pushed the fix-io-copy-vec branch from 0884daf to 78aa5e5 Compare November 4, 2023 15:11

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 6, 2023

bors merged commit 7a892ab into rust-lang:master Nov 6, 2023
12 checks passed

rustbot added this to the 1.75.0 milestone Nov 6, 2023

github-actions bot mentioned this pull request Nov 6, 2023

Automated pull from upstream master ferrocene/ferrocene#84

Merged

bonsairobo mentioned this pull request Nov 8, 2023

Bad frame decompression performance regression for large data sets in Rust 1.73 PSeitz/lz4_flex#147

Closed

the8472 added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 9, 2023

Mark-Simulacrum mentioned this pull request Nov 13, 2023

[stable] Prepare 1.74.0 release #117843

Merged

Amanieu added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Nov 15, 2023

rustbot removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 17, 2023

cuviper modified the milestones: 1.75.0, 1.74.0 Nov 17, 2023

the8472 mentioned this pull request Nov 24, 2023

unify read_to_end and io::copy impls for reading into a Vec #118222

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix excessive initialization and reads beyond EOF in `io::copy(_, Vec<u8>)` specialization #117576

Fix excessive initialization and reads beyond EOF in `io::copy(_, Vec<u8>)` specialization #117576

the8472 commented Nov 4, 2023

rustbot commented Nov 4, 2023

This comment has been minimized.

Mark-Simulacrum commented Nov 4, 2023

bors commented Nov 4, 2023

bors commented Nov 6, 2023

bors commented Nov 6, 2023

rust-timer commented Nov 6, 2023

a1phyr commented Nov 9, 2023

the8472 commented Nov 9, 2023

the8472 commented Nov 9, 2023

cuviper commented Nov 17, 2023

Fix excessive initialization and reads beyond EOF in io::copy(_, Vec<u8>) specialization #117576

Fix excessive initialization and reads beyond EOF in io::copy(_, Vec<u8>) specialization #117576

Conversation

the8472 commented Nov 4, 2023

rustbot commented Nov 4, 2023

This comment has been minimized.

Mark-Simulacrum commented Nov 4, 2023

bors commented Nov 4, 2023

bors commented Nov 6, 2023

bors commented Nov 6, 2023

rust-timer commented Nov 6, 2023

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

a1phyr commented Nov 9, 2023

the8472 commented Nov 9, 2023

the8472 commented Nov 9, 2023

cuviper commented Nov 17, 2023

Fix excessive initialization and reads beyond EOF in `io::copy(_, Vec<u8>)` specialization #117576

Fix excessive initialization and reads beyond EOF in `io::copy(_, Vec<u8>)` specialization #117576