Avoid more NonNull-raw-NonNull roundtrips in Vec #123835

saethlin · 2024-04-12T01:18:52Z

r? the8472

The standard library in general has a lot of these round-trips from niched types to their raw innards and back. Such round-trips have overhead in debug builds since #120594. I removed some such round-trips in that initial PR and I've been meaning to come back and hunt down more such examples (this is the last item on #120848).

saethlin · 2024-04-12T01:29:51Z

@bors try @rust-timer queue

bors · 2024-04-12T01:31:01Z

⌛ Trying commit e4c567e with merge 7d882e6...

Avoid more NonNull-raw-NonNull roundtrips in Vec r? the8472 The standard library in general has a lot of these round-trips from niched types to their raw innards and back. Such round-trips have overhead in debug builds since rust-lang#120594. I removed some such round-trips in that initial PR and I've been meaning to come back and hunt down more such examples (this is the last item on rust-lang#120848).

bors · 2024-04-12T03:03:47Z

☀️ Try build successful - checks-actions
Build commit: 7d882e6 (7d882e65737d22b40e2b8c565bd429f08461e0fc)

rust-timer · 2024-04-12T04:37:25Z

Finished benchmarking commit (7d882e6): 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)	3.4%	[2.2%, 4.2%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.8%	[-5.0%, -2.4%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.6%	[-5.0%, 4.2%]	9

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.3%	[-2.3%, -2.3%]	2
Improvements ✅ (secondary)	-1.5%	[-1.8%, -1.1%]	3
All ❌✅ (primary)	-2.3%	[-2.3%, -2.3%]	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.0%	[0.0%, 0.1%]	9
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.7%, -0.0%]	16
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.7%, 0.1%]	25

Bootstrap: 676.479s -> 675.803s (-0.10%)
Artifact size: 316.06 MiB -> 315.98 MiB (-0.03%)

the8472 · 2024-04-12T18:14:48Z

Binary sizes seem to be a wash. Instructions improvements are extremely tiny, below individual significance thresholds. Wall-times look positive but that's just reverting a spike from the previous run.

From the PR description it sounds like bigger improvements were expected.

saethlin · 2024-04-12T19:14:22Z

From the PR description it sounds like bigger improvements were expected.

Sorry I gave that impression. The entire envelope here if precondition checks were reduced to 0 is 1.7% in instruction count, and this change reduces the number of precondition checks in a full build of exa-0.10.1 from 1170 to 1141. I plan on whittling away at that number of compiled checks over a number of PRs, and I'd prefer they all stand on their own, but I want to leave a trail so that if the sum of all my efforts reduces the overhead of the precondition checks feature, people know that was due to a deliberate effort.

If you only want to merge this if it provides measurable perf changes on its own, I could sit on this until I can bundle it with a lot of other unrelated changes so that in total they are measurable. I don't like working that way but I know some people prefer it.

the8472

No, it's fine, I misunderstood the expectation then. The results aren't negative.

Just a few minor things then

the8472 · 2024-04-12T19:37:17Z

library/alloc/src/raw_vec.rs

@@ -259,6 +259,11 @@ impl<T, A: Allocator> RawVec<T, A> {
        Self { ptr: unsafe { Unique::new_unchecked(ptr) }, cap, alloc }
    }

+    pub(crate) unsafe fn from_nonnull_in(ptr: NonNull<T>, capacity: usize, alloc: A) -> Self {


It's an unsafe method, so it should at least point to its sibling regarding the Safety section.

the8472 · 2024-04-12T19:41:11Z

library/alloc/src/vec/mod.rs

+
+    #[inline]
+    #[cfg(not(no_global_oom_handling))] // required by tests/run-make/alloc-no-oom-handling
+    pub(crate) unsafe fn from_nonnull(ptr: NonNull<T>, length: usize, capacity: usize) -> Self {


Ditto about Safety section/reference to sibling method.

the8472 · 2024-04-12T19:41:15Z

library/alloc/src/vec/in_place_collect.rs

            let Ok(reallocated) = result else { handle_alloc_error(new_layout) };
-            dst_buf = reallocated.as_ptr() as *mut T;
+            dst_buf = reallocated.cast();


Can you add the <T>? Since this method deals with an input and an output type it's nice to have those little reminders with which one of the two we're dealing again in various places.

the8472

@bors r+ rollup

the8472 · 2024-04-12T22:39:26Z

looks like the bot doesn't read review comments.

@bors r+ rollup

bors · 2024-04-12T22:39:29Z

📌 Commit f7d54fa has been approved by the8472

It is now in the queue for this repository.

Rollup of 4 pull requests Successful merges: - rust-lang#123835 (Avoid more NonNull-raw-NonNull roundtrips in Vec) - rust-lang#123868 (Stabilize (const_)slice_ptr_len and (const_)slice_ptr_is_empty_nonnull) - rust-lang#123872 (Fix Pietro's entry in the mailmap) - rust-lang#123873 (Merge cuviper in the mailmap) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#123835 - saethlin:vec-from-nonnull, r=the8472 Avoid more NonNull-raw-NonNull roundtrips in Vec r? the8472 The standard library in general has a lot of these round-trips from niched types to their raw innards and back. Such round-trips have overhead in debug builds since rust-lang#120594. I removed some such round-trips in that initial PR and I've been meaning to come back and hunt down more such examples (this is the last item on rust-lang#120848).

rustbot assigned the8472 Apr 12, 2024

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 Apr 12, 2024

This was referenced Apr 12, 2024

Totally-not-a-tracking-issue for UB-detecting debug assertions in the standard library #120848

Open

Disable assert_unsafe_precondition to measure compile-time cost #123679

Closed

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 Apr 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 Apr 12, 2024

saethlin force-pushed the vec-from-nonnull branch from e4c567e to e575007 Compare April 12, 2024 04:49

This comment has been minimized.

Sign in to view

saethlin force-pushed the vec-from-nonnull branch from e575007 to 0f2d18f Compare April 12, 2024 15:08

the8472 requested changes Apr 12, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 12, 2024

Avoid more NonNull-raw-NonNull roundtrips in Vec

f7d54fa

saethlin force-pushed the vec-from-nonnull branch from 0f2d18f to f7d54fa Compare April 12, 2024 22:14

the8472 approved these changes Apr 12, 2024

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 12, 2024

jhpratt mentioned this pull request Apr 13, 2024

Rollup of 4 pull requests #123884

Merged

bors merged commit 8533144 into rust-lang:master Apr 13, 2024
11 checks passed

rustbot added this to the 1.79.0 milestone Apr 13, 2024

saethlin deleted the vec-from-nonnull branch April 13, 2024 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid more NonNull-raw-NonNull roundtrips in Vec #123835

Avoid more NonNull-raw-NonNull roundtrips in Vec #123835

saethlin commented Apr 12, 2024

saethlin commented Apr 12, 2024

This comment has been minimized.

bors commented Apr 12, 2024

This comment has been minimized.

bors commented Apr 12, 2024

This comment has been minimized.

rust-timer commented Apr 12, 2024

This comment has been minimized.

the8472 commented Apr 12, 2024

saethlin commented Apr 12, 2024 •

edited

Loading

the8472 left a comment

the8472 Apr 12, 2024

the8472 Apr 12, 2024

the8472 Apr 12, 2024

the8472 left a comment

the8472 commented Apr 12, 2024

bors commented Apr 12, 2024

Avoid more NonNull-raw-NonNull roundtrips in Vec #123835

Avoid more NonNull-raw-NonNull roundtrips in Vec #123835

Conversation

saethlin commented Apr 12, 2024

saethlin commented Apr 12, 2024

This comment has been minimized.

bors commented Apr 12, 2024

This comment has been minimized.

bors commented Apr 12, 2024

This comment has been minimized.

rust-timer commented Apr 12, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

This comment has been minimized.

the8472 commented Apr 12, 2024

saethlin commented Apr 12, 2024 • edited Loading

the8472 left a comment

Choose a reason for hiding this comment

the8472 Apr 12, 2024

Choose a reason for hiding this comment

the8472 Apr 12, 2024

Choose a reason for hiding this comment

the8472 Apr 12, 2024

Choose a reason for hiding this comment

the8472 left a comment

Choose a reason for hiding this comment

the8472 commented Apr 12, 2024

bors commented Apr 12, 2024

saethlin commented Apr 12, 2024 •

edited

Loading