Don't use mem::zeroed in vec::IntoIter #120952

saethlin · 2024-02-11T22:19:36Z

mem::zeroed is not a trivial function. Maybe it was once, but now it involves multiple locals, copies, and an intrinsic that gets monomorphized into a call to panic_nounwind for iterators of types like Vec<&T>. Of course all that complexity is trivially optimized out, but generating a bunch of IR where we don't need to just so we can optimize it away later is silly.

saethlin · 2024-02-11T22:25:40Z

@bors try @rust-timer queue

Don't use mem::zeroed in vec::IntoIter r? `@ghost`

bors · 2024-02-11T22:26:49Z

⌛ Trying commit 35a9a09 with merge 218b423...

bors · 2024-02-11T23:54:04Z

☀️ Try build successful - checks-actions
Build commit: 218b423 (218b423933a990ea9b14a658106cbc7180a2ac1b)

bors · 2024-02-11T23:54:05Z

☀️ Try build successful - checks-actions
Build commit: 218b423 (218b423933a990ea9b14a658106cbc7180a2ac1b)

rust-timer · 2024-02-12T01:18:00Z

Finished benchmarking commit (218b423): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

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

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

Binary size

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.1%	[0.0%, 0.3%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.4%, -0.0%]	31
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.4%, 0.3%]	34

Bootstrap: 664.283s -> 663.002s (-0.19%)
Artifact size: 308.29 MiB -> 308.29 MiB (-0.00%)

saethlin · 2024-02-12T01:20:08Z

@bors try @rust-timer queue

bors · 2024-02-12T01:21:17Z

⌛ Trying commit 78a4d71 with merge 0183a22...

Don't use mem::zeroed in vec::IntoIter `mem::zeroed` is not a trivial function. Maybe it was once, but now it involves multiple locals, copies, and an intrinsic that gets monomorphized into a call to `panic_nounwind` for iterators of types like `Vec<&T>`. Of course all that complexity is trivially optimized out, but generating a bunch of IR where we don't need to just so we can optimize it away later is silly. r? `@ghost`

bors · 2024-02-12T02:50:11Z

☀️ Try build successful - checks-actions
Build commit: 0183a22 (0183a224a76c9b4106c34175108ff9a671c6dc52)

rust-timer · 2024-02-12T04:06:24Z

Finished benchmarking commit (0183a22): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

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

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.0%	[-19.2%, -2.2%]	6
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-6.0%	[-19.2%, -2.2%]	6

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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.5%	[-1.9%, -1.1%]	2
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	-1.5%	[-1.9%, -1.1%]	2

Binary size

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.3%	[0.0%, 0.8%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.4%, -0.1%]	35
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.4%, 0.8%]	38

Bootstrap: 666.51s -> 663.607s (-0.44%)
Artifact size: 308.32 MiB -> 308.30 MiB (-0.01%)

the8472 · 2024-02-12T13:16:20Z

library/alloc/src/vec/into_iter.rs

-                self.ptr = unsafe { old.add(1) };
-
-                Some(unsafe { ptr::read(old.as_ptr()) })
+            if self.ptr == unsafe { NonNull::new_unchecked(self.end as *mut T) } {


if the goal is IR reduction, well, NonNull::new_unchecked does contain some assert_unsafe_precondition! machinery too, the non_null! macro avoids that.

saethlin · 2024-02-12T23:45:13Z

@bors try @rust-timer queue

bors · 2024-02-12T23:46:23Z

⌛ Trying commit eaa3133 with merge 491feef...

Don't use mem::zeroed in vec::IntoIter `mem::zeroed` is not a trivial function. Maybe it was once, but now it involves multiple locals, copies, and an intrinsic that gets monomorphized into a call to `panic_nounwind` for iterators of types like `Vec<&T>`. Of course all that complexity is trivially optimized out, but generating a bunch of IR where we don't need to just so we can optimize it away later is silly. r? `@ghost`

bors · 2024-02-13T01:14:41Z

☀️ Try build successful - checks-actions
Build commit: 491feef (491feef8f7714821d702726a1652d7196845cbad)

rust-timer · 2024-02-13T06:03:29Z

Finished benchmarking commit (491feef): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -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)	2.9%	[2.9%, 2.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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

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

Binary size

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.2%	[0.0%, 0.5%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.1%]	36
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.2%, 0.5%]	39

Bootstrap: 664.301s -> 664.247s (-0.01%)
Artifact size: 308.35 MiB -> 308.31 MiB (-0.01%)

saethlin · 2024-02-15T23:22:33Z

The small binary size reduction in debug builds is the only real evidence that this PR is working as intended.

So I'm happy to apply whatever cleanups in this module seem sensible, because I think tidying up the code is the primary outcome of this now.

saethlin · 2024-02-15T23:22:44Z

r? the8472

library/alloc/src/vec/into_iter.rs

the8472 · 2024-02-15T23:40:36Z

library/alloc/src/vec/into_iter.rs

-                let new_end = unsafe { non_null!(self.end, T).sub(1) };
-                *non_null!(mut self.end, T) = new_end;
-
-                Some(unsafe { ptr::read(new_end.as_ptr()) })


next_back isn't covered by codegen/vec-iter. can you check if it still has the nonnull attribute?

While working on this, I think I realized that the transmute strategy I was trying to adopt was working by sheer luck. I thought I was getting !nonnull from the transmute range assumes, but we don't insert those for pointers. I'm looking into that, but not in this PR.

bors · 2024-02-16T09:53:24Z

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

the8472 · 2024-02-17T10:55:47Z

Looks like the use of zeroed() was introduced in #53804. Back then we didn't do the length-tracking for ZSTs with the end pointer. so ptr ended up unaligned. So that doesn't apply anymore.

@bors r+ rollup

bors · 2024-02-17T10:55:50Z

📌 Commit 7c2db70 has been approved by the8472

It is now in the queue for this repository.

Don't use mem::zeroed in vec::IntoIter `mem::zeroed` is not a trivial function. Maybe it was once, but now it involves multiple locals, copies, and an intrinsic that gets monomorphized into a call to `panic_nounwind` for iterators of types like `Vec<&T>`. Of course all that complexity is trivially optimized out, but generating a bunch of IR where we don't need to just so we can optimize it away later is silly.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#120952 (Don't use mem::zeroed in vec::IntoIter) - rust-lang#121079 (distribute tool documentations and avoid file conflicts on `x install`) - rust-lang#121085 (errors: only eagerly translate subdiagnostics) - rust-lang#121091 (use build.rustc config and skip-stage0-validation flag) - rust-lang#121193 (Use fulfillment in next trait solver coherence) - rust-lang#121209 (Make `CodegenBackend::join_codegen` infallible.) - rust-lang#121210 (Fix `cfg(target_abi = "sim")` on `i386-apple-ios`) - rust-lang#121228 (create stamp file for clippy) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#120952 (Don't use mem::zeroed in vec::IntoIter) - rust-lang#121085 (errors: only eagerly translate subdiagnostics) - rust-lang#121091 (use build.rustc config and skip-stage0-validation flag) - rust-lang#121149 (Fix typo in VecDeque::handle_capacity_increase() doc comment.) - rust-lang#121193 (Use fulfillment in next trait solver coherence) - rust-lang#121209 (Make `CodegenBackend::join_codegen` infallible.) - rust-lang#121210 (Fix `cfg(target_abi = "sim")` on `i386-apple-ios`) - rust-lang#121228 (create stamp file for clippy) - rust-lang#121231 (remove a couple of redundant clones) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#120952 - saethlin:vec-into-iter, r=the8472 Don't use mem::zeroed in vec::IntoIter `mem::zeroed` is not a trivial function. Maybe it was once, but now it involves multiple locals, copies, and an intrinsic that gets monomorphized into a call to `panic_nounwind` for iterators of types like `Vec<&T>`. Of course all that complexity is trivially optimized out, but generating a bunch of IR where we don't need to just so we can optimize it away later is silly.

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 Feb 11, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 11, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2024

Auto merge of rust-lang#120952 - saethlin:vec-into-iter, r=<try>

218b423

Don't use mem::zeroed in vec::IntoIter r? `@ghost`

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 12, 2024

saethlin force-pushed the vec-into-iter branch from 35a9a09 to 78a4d71 Compare February 12, 2024 01:19

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 12, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 12, 2024

the8472 reviewed Feb 12, 2024

View reviewed changes

saethlin force-pushed the vec-into-iter branch from 78a4d71 to eaa3133 Compare February 12, 2024 23:44

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 12, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 13, 2024

rustbot assigned the8472 Feb 15, 2024

saethlin marked this pull request as ready for review February 15, 2024 23:22

the8472 reviewed Feb 15, 2024

View reviewed changes

library/alloc/src/vec/into_iter.rs Show resolved Hide resolved

the8472 reviewed Feb 15, 2024

View reviewed changes

saethlin force-pushed the vec-into-iter branch 2 times, most recently from 9c5a27f to 83a9d95 Compare February 16, 2024 05:03

Don't use mem::zeroed in vec::IntoIter

7c2db70

saethlin force-pushed the vec-into-iter branch from 83a9d95 to 7c2db70 Compare February 16, 2024 15:46

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 Feb 17, 2024

matthiaskrgr mentioned this pull request Feb 17, 2024

Rollup of 9 pull requests #121234

Closed

matthiaskrgr mentioned this pull request Feb 17, 2024

Rollup of 8 pull requests #121235

Closed

matthiaskrgr mentioned this pull request Feb 17, 2024

Rollup of 9 pull requests #121240

Merged

bors merged commit 5997286 into rust-lang:master Feb 17, 2024
11 checks passed

rustbot added this to the 1.78.0 milestone Feb 17, 2024

saethlin deleted the vec-into-iter branch February 17, 2024 20:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use mem::zeroed in vec::IntoIter #120952

Don't use mem::zeroed in vec::IntoIter #120952

saethlin commented Feb 11, 2024 •

edited

Loading

saethlin commented Feb 11, 2024

This comment has been minimized.

bors commented Feb 11, 2024

bors commented Feb 11, 2024

bors commented Feb 11, 2024

This comment has been minimized.

rust-timer commented Feb 12, 2024

saethlin commented Feb 12, 2024

This comment has been minimized.

bors commented Feb 12, 2024

bors commented Feb 12, 2024

This comment has been minimized.

rust-timer commented Feb 12, 2024

the8472 Feb 12, 2024

saethlin commented Feb 12, 2024

This comment has been minimized.

bors commented Feb 12, 2024

bors commented Feb 13, 2024

This comment has been minimized.

rust-timer commented Feb 13, 2024

saethlin commented Feb 15, 2024

saethlin commented Feb 15, 2024

the8472 Feb 15, 2024

saethlin Feb 16, 2024

bors commented Feb 16, 2024

the8472 commented Feb 17, 2024

bors commented Feb 17, 2024

Don't use mem::zeroed in vec::IntoIter #120952

Don't use mem::zeroed in vec::IntoIter #120952

Conversation

saethlin commented Feb 11, 2024 • edited Loading

saethlin commented Feb 11, 2024

This comment has been minimized.

bors commented Feb 11, 2024

bors commented Feb 11, 2024

bors commented Feb 11, 2024

This comment has been minimized.

rust-timer commented Feb 12, 2024

Overall result: no relevant changes - no action needed

saethlin commented Feb 12, 2024

This comment has been minimized.

bors commented Feb 12, 2024

bors commented Feb 12, 2024

This comment has been minimized.

rust-timer commented Feb 12, 2024

Overall result: no relevant changes - no action needed

Choose a reason for hiding this comment

saethlin commented Feb 12, 2024

This comment has been minimized.

bors commented Feb 12, 2024

bors commented Feb 13, 2024

This comment has been minimized.

rust-timer commented Feb 13, 2024

Overall result: ❌ regressions - no action needed

saethlin commented Feb 15, 2024

saethlin commented Feb 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 16, 2024

the8472 commented Feb 17, 2024

bors commented Feb 17, 2024

saethlin commented Feb 11, 2024 •

edited

Loading