Simplify manual ptr arithmetic in slice::Iter with ptr_sub #106393

the8472 · 2023-01-03T01:46:11Z

The old code was introduced in #61885, which predates the ptr_sub method and underlying intrinsic. The codegen test still passes.

r? @scottmcm

rustbot · 2023-01-03T01:46:18Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

scottmcm · 2023-01-14T01:08:09Z

Oh, thanks! This was the original motivating example for #95837, but I didn't want to change something as critical as slice iterators in that PR. r=me assuming that this doesn't have some weird perf implication

@bors try @rust-timer queue

bors · 2023-01-14T01:09:11Z

⌛ Trying commit e7e9e477a4ffffe49e0ee4657626b18c71700026 with merge 018625f89e1b5bf3ce3585225ac9ca701f9bf0f1...

bors · 2023-01-14T03:21:08Z

☀️ Try build successful - checks-actions
Build commit: 018625f89e1b5bf3ce3585225ac9ca701f9bf0f1 (018625f89e1b5bf3ce3585225ac9ca701f9bf0f1)

rust-timer · 2023-01-14T06:21:33Z

Finished benchmarking commit (018625f89e1b5bf3ce3585225ac9ca701f9bf0f1): 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)	1.0%	[1.0%, 1.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
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)	3.0%	[0.0%, 6.1%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.2%	[-2.7%, -1.4%]	3
Improvements ✅ (secondary)	-1.7%	[-2.7%, -0.7%]	2
All ❌✅ (primary)	-0.1%	[-2.7%, 6.1%]	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)	1.8%	[1.8%, 1.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.0%	[-3.0%, -3.0%]	1
All ❌✅ (primary)	1.8%	[1.8%, 1.8%]	1

scottmcm · 2023-01-14T06:39:30Z

Perf looks essentially neutral, the one change is because of the codegen schedule that appears to actually be shorter overall, despite the extra instructions.

@bors rollup=maybe

Still needs a rebase, though, @the8472. Feel free to r=me after that.
@rustbot author

the8472 · 2023-01-14T13:34:19Z

@bors r=scottmcm

bors · 2023-01-14T13:34:20Z

📌 Commit e31d73ca7ef44e855b3d55c9ab6749ba5f78be10 has been approved by scottmcm

It is now in the queue for this repository.

bors · 2023-01-15T00:06:50Z

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

the8472 · 2023-01-15T11:40:48Z

@bors r=scottmcm

bors · 2023-01-15T11:40:50Z

📌 Commit 3a3d2b4c9c8469272278f8cf602de7c16d39b3fb has been approved by scottmcm

It is now in the queue for this repository.

bors · 2023-01-15T14:52:16Z

⌛ Testing commit 3a3d2b4c9c8469272278f8cf602de7c16d39b3fb with merge d1fc44b7b575d0f3dc7bcb492719fe7cf71d5aad...

bors · 2023-01-15T15:07:21Z

💔 Test failed - checks-actions

the8472 · 2023-01-15T15:45:40Z

Ok, this probably isn't due to the llvm version then. I see that sub_ptr has debug asserts that the old code didn't have.

the8472 · 2023-01-15T18:13:04Z

@bors r=scottmcm

bors · 2023-01-15T18:13:06Z

📌 Commit 9db0134 has been approved by scottmcm

It is now in the queue for this repository.

bors · 2023-01-15T18:39:42Z

⌛ Testing commit 9db0134 with merge 9e75ddd...

bors · 2023-01-15T21:35:43Z

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 9e75ddd to master...

rust-timer · 2023-01-15T22:54:59Z

Finished benchmarking commit (9e75ddd): comparison URL.

Overall result: ❌ regressions - 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)	1.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	0.4%	[0.3%, 0.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[1.1%, 1.1%]	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)	3.0%	[0.0%, 5.9%]	2
Regressions ❌ (secondary)	2.3%	[2.1%, 2.5%]	2
Improvements ✅ (primary)	-2.8%	[-2.8%, -2.8%]	1
Improvements ✅ (secondary)	-4.4%	[-4.4%, -4.4%]	1
All ❌✅ (primary)	1.1%	[-2.8%, 5.9%]	3

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

RalfJung · 2023-01-16T16:58:30Z

library/core/src/slice/iter/macros.rs

+            // To get rid of some bounds checks (see `position`), we use ptr_sub instead of
+            // offset_from (Tested by `codegen/slice-position-bounds-check`.)
+            // SAFETY: by the type invariant pointers are aligned and `start <= end`
+            unsafe { $self.end.sub_ptr(start.as_ptr()) }


This safety comment is incomplete. sub_ptr (via the offset_from safety requirements) requires the pointers to point to the same allocation, and they must be dereferenceable, i.e., point to allocated memory. The latter is not currently always the case, though whether that is a bug in this function or a bug in whatever creates those dangling raw pointers is unclear.

See Zulip

RalfJung · 2023-01-16T16:59:28Z

library/core/src/slice/iter/macros.rs

-        let size = size_from_ptr(start.as_ptr());
-        if size == 0 {
-            // This _cannot_ use `unchecked_sub` because we depend on wrapping
+        if T::IS_ZST {


This is odd, where is T bound? Does this macro assume that there is a T in scope wherever it is used (violating hygiene)? That at least warrants a comment.

The macro is only used in another macro (in the same file) which defines T

rustbot assigned scottmcm Jan 3, 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 Jan 3, 2023

the8472 force-pushed the use-ptr-sub branch from 30da7c0 to e7e9e47 Compare January 3, 2023 02:17

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 Jan 14, 2023

scottmcm 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 Jan 14, 2023

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 Jan 14, 2023

the8472 force-pushed the use-ptr-sub branch from e7e9e47 to e31d73c Compare January 14, 2023 12:23

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 Jan 14, 2023

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 15, 2023

the8472 force-pushed the use-ptr-sub branch 2 times, most recently from b91b493 to 3a3d2b4 Compare January 15, 2023 11:24

bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 15, 2023

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 15, 2023

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

This comment has been minimized.

Sign in to view

replace manual ptr arithmetic with ptr_sub

9db0134

the8472 force-pushed the use-ptr-sub branch from 3a3d2b4 to 9db0134 Compare January 15, 2023 16:38

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 Jan 15, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 15, 2023

bors merged commit 9e75ddd into rust-lang:master Jan 15, 2023

rustbot added this to the 1.68.0 milestone Jan 15, 2023

RalfJung reviewed Jan 16, 2023

View reviewed changes

scottmcm mentioned this pull request Mar 22, 2023

Tracking Issue for offset_from_unsigned (feature ptr_sub_ptr) #95892

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify manual ptr arithmetic in slice::Iter with ptr_sub #106393

Simplify manual ptr arithmetic in slice::Iter with ptr_sub #106393

the8472 commented Jan 3, 2023

rustbot commented Jan 3, 2023

scottmcm commented Jan 14, 2023

This comment has been minimized.

bors commented Jan 14, 2023

bors commented Jan 14, 2023

This comment has been minimized.

rust-timer commented Jan 14, 2023

scottmcm commented Jan 14, 2023

the8472 commented Jan 14, 2023

bors commented Jan 14, 2023

bors commented Jan 15, 2023

the8472 commented Jan 15, 2023

bors commented Jan 15, 2023

bors commented Jan 15, 2023

bors commented Jan 15, 2023

the8472 commented Jan 15, 2023

This comment has been minimized.

the8472 commented Jan 15, 2023

bors commented Jan 15, 2023

bors commented Jan 15, 2023

bors commented Jan 15, 2023

rust-timer commented Jan 15, 2023

RalfJung Jan 16, 2023 •

edited

Loading

RalfJung Jan 16, 2023

the8472 Jan 16, 2023

Simplify manual ptr arithmetic in slice::Iter with ptr_sub #106393

Simplify manual ptr arithmetic in slice::Iter with ptr_sub #106393

Conversation

the8472 commented Jan 3, 2023

rustbot commented Jan 3, 2023

scottmcm commented Jan 14, 2023

This comment has been minimized.

bors commented Jan 14, 2023

bors commented Jan 14, 2023

This comment has been minimized.

rust-timer commented Jan 14, 2023

Overall result: ❌ regressions - no action needed

scottmcm commented Jan 14, 2023

the8472 commented Jan 14, 2023

bors commented Jan 14, 2023

bors commented Jan 15, 2023

the8472 commented Jan 15, 2023

bors commented Jan 15, 2023

bors commented Jan 15, 2023

bors commented Jan 15, 2023

the8472 commented Jan 15, 2023

This comment has been minimized.

the8472 commented Jan 15, 2023

bors commented Jan 15, 2023

bors commented Jan 15, 2023

bors commented Jan 15, 2023

rust-timer commented Jan 15, 2023

Overall result: ❌ regressions - no action needed

RalfJung Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Jan 16, 2023 •

edited

Loading