Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use NonNull in slice::Iter and slice::IterMut. #67588

Merged
merged 1 commit into from Dec 28, 2019

Conversation

@Kixunil
Copy link
Contributor

Kixunil commented Dec 24, 2019

ptr of slice::Iter and slice::IterMut can never be null, but this
fact wasn't exploited for layout optimizations. By changing ptr from
*<mutability> T to NonNull<T>, the compiler can now optimize layout
of Option<Iter<'a, T>>.

Closes #67228

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 24, 2019

r? @withoutboats

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

@@ -3215,7 +3223,7 @@ macro_rules! iterator {
fn next(&mut self) -> Option<$elem> {
// could be implemented with slices, but this avoids bounds checks
unsafe {
assume(!self.ptr.is_null());
assume(!self.ptr.as_ptr().is_null());

This comment has been minimized.

Copy link
@ranma42

ranma42 Dec 24, 2019

Contributor

This is now implied by the NonNull type, right? Should we remove this assume?

This comment has been minimized.

Copy link
@Kixunil

Kixunil Dec 24, 2019

Author Contributor

My understanding is it's an optimization when it comes to code generation, not layout optimization. But maybe rustc is smart enough about that? I'm not that familiar with how NonNull works.

This comment has been minimized.

Copy link
@Kixunil

Kixunil Dec 24, 2019

Author Contributor

Looks like we could remove it. Is that example good enough evidence? Or should we reach out to someone who knows better?

This comment has been minimized.

Copy link
@rkruppe

rkruppe Dec 25, 2019

Member

The "this pointer is non-null" information added by rustc to LLVM IR automatically is limited to particular positions (function parameters, function return values, loads from memory) which can get eliminated by optimizations (such as inlining and SROA) before they have served their purpose. In contrast, assumes stay around basically for the entire optimization pipeline. So I am not confident that we can remove this assume without regressions. You're welcome to try to evaluate that on real world code (the example linked above is far too isolated, I would recommend history spelunking to find the benchmarks that motivated adding the assume in the first place) but I would recommend doing that in a separate PR.

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Dec 24, 2019

@@ -3467,7 +3470,7 @@ impl<T> AsRef<[T]> for Iter<'_, T> {
/// [slices]: ../../std/primitive.slice.html
#[stable(feature = "rust1", since = "1.0.0")]
pub struct IterMut<'a, T: 'a> {
ptr: *mut T,
ptr: NonNull<T>,
end: *mut T, // If T is a ZST, this is actually ptr+len. This encoding is picked so that
// ptr == end is a quick test for the Iterator being empty, that works
// for both ZST and non-ZST.

This comment has been minimized.

Copy link
@petertodd

petertodd Dec 24, 2019

Contributor

Can they both be NonNull<T>? IIUC it's an invariant that ptr <= end, so a null end is invalid.

We don't use multiple niches yet, but a future optimization might, and it could help communicate the invariant better.

This comment has been minimized.

Copy link
@Kixunil

Kixunil Dec 24, 2019

Author Contributor

According to a comment further down end might be 0 in case of ZST.

Thanks for review, BTW! :)

This comment has been minimized.

Copy link
@petertodd

petertodd Dec 24, 2019

Contributor

Ah of course: a ZST slice can have an arbitrary starting pointer, and len is encoded as the wrapping subtraction of end - start. Thus a slice of length 1 with the max-int pointer results in end = 0.

You know, now that we have unions a simpler way to do all this could be:

union LenOrEnd<T> {
    end: NonNull<T>,
    len: usize,
}

pub struct IterMut<'a, T> {
    marker: PhantomData<&'a mut T>,
    start: NonNull<T>,
    len_or_end: LenOrEnd<T>,
}

A union of SizedIterMut and ZeroSizedIterMut is another possibility. Except that currently union's don't participate in layout optimization. :/

This comment has been minimized.

Copy link
@petertodd

petertodd Dec 24, 2019

Contributor

I'll bet you this will have slightly better performance too by representing len directly rather than needing to do arithmetic to derive it.

This comment has been minimized.

Copy link
@Kixunil

Kixunil Dec 24, 2019

Author Contributor

I was thinking that the code might be improved in different ways too. I just think that we should go with smaller increments. The PR already improves something, new improvements could be in the next PR.

Anyway, I think the layout would have to be determined in type, not in union in order to enable optimizations. I'm not an expert but I suspect that making rustc smart enough to understand which part of the union is used is based on type and conditionally optimize layout would be very difficult.

This comment has been minimized.

Copy link
@petertodd

petertodd Dec 24, 2019

Contributor

Yeah, I agree that an incremental improvement is fine right now.

The above does get the NonNull<T> layout optimization as the NonNull<T> is in the struct, rather than the union: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=82eb70c78825be8dd5e48fd5d8d521cc

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented Dec 25, 2019

I like this diff as-is, removing assumes can be a follow up. Please rebase (we avoid merge commits) and squash so I can approve it.

@Kixunil Kixunil force-pushed the Kixunil:nonnull-slice-iter branch from 9a654c8 to 047ff0b Dec 25, 2019
@Kixunil

This comment has been minimized.

Copy link
Contributor Author

Kixunil commented Dec 25, 2019

Thanks for explanation and instructions! I hope I cleaned it up correctly. I like the diff as well. I don't have enough expertise to be confident about removing assumes, so don't expect the next PR doing it from me. I'll focus on things I'm better at. :)

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented Dec 25, 2019

Alright, thanks a lot!

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 25, 2019

📌 Commit 047ff0b has been approved by rkruppe

@Kixunil

This comment has been minimized.

Copy link
Contributor Author

Kixunil commented Dec 25, 2019

I'm happy to have helped, thank you too!

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Dec 26, 2019
Use NonNull in slice::Iter and slice::IterMut.

`ptr` of `slice::Iter` and `slice::IterMut` can never be null, but this
fact wasn't exploited for layout optimizations. By changing `ptr` from
`*<mutability> T` to `NonNull<T>`, the compiler can now optimize layout
of `Option<Iter<'a, T>>`.

Closes rust-lang#67228
bors added a commit that referenced this pull request Dec 26, 2019
Rollup of 7 pull requests

Successful merges:

 - #67576 (reuse `capacity` variable in slice::repeat)
 - #67588 (Use NonNull in slice::Iter and slice::IterMut.)
 - #67598 (Fix ICE / miscompilation when inlining simd shuffle intrinsic in MIR.)
 - #67602 (Use issue = "none" instead of "0" in intrinsics)
 - #67604 (Add Scalar::to_(u|i)16 methods)
 - #67605 (tidy: change msdn links to newer locations)
 - #67617 (Remove `compiler_builtins_lib` documentation)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 26, 2019

⌛️ Testing commit 047ff0b with merge 9b345cb...

bors added a commit that referenced this pull request Dec 26, 2019
Use NonNull in slice::Iter and slice::IterMut.

`ptr` of `slice::Iter` and `slice::IterMut` can never be null, but this
fact wasn't exploited for layout optimizations. By changing `ptr` from
`*<mutability> T` to `NonNull<T>`, the compiler can now optimize layout
of `Option<Iter<'a, T>>`.

Closes #67228
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 26, 2019

The job i686-apple of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 26, 2019

💔 Test failed - checks-azure

src/libcore/slice/mod.rs Outdated Show resolved Hide resolved
`ptr` of `slice::Iter` and `slice::IterMut` can never be null, but this
fact wasn't exploited for layout optimizations. By changing `ptr` from
`*<mutability> T` to `NonNull<T>`, the compiler can now optimize layout
of `Option<Iter<'a, T>>`.
@Kixunil Kixunil force-pushed the Kixunil:nonnull-slice-iter branch from 5e84631 to 2c796ee Dec 26, 2019
@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented Dec 27, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 27, 2019

📌 Commit 2c796ee has been approved by rkruppe

oli-obk added a commit to oli-obk/rust that referenced this pull request Dec 27, 2019
Use NonNull in slice::Iter and slice::IterMut.

`ptr` of `slice::Iter` and `slice::IterMut` can never be null, but this
fact wasn't exploited for layout optimizations. By changing `ptr` from
`*<mutability> T` to `NonNull<T>`, the compiler can now optimize layout
of `Option<Iter<'a, T>>`.

Closes rust-lang#67228
bors added a commit that referenced this pull request Dec 28, 2019
Rollup of 15 pull requests

Successful merges:

 - #65244 (add IntoFuture trait and support for await)
 - #67576 (reuse `capacity` variable in slice::repeat)
 - #67588 (Use NonNull in slice::Iter and slice::IterMut.)
 - #67594 (Update libc to 0.2.66)
 - #67602 (Use issue = "none" instead of "0" in intrinsics)
 - #67604 (Add Scalar::to_(u|i)16 methods)
 - #67617 (Remove `compiler_builtins_lib` documentation)
 - #67621 (Use the correct type for static qualifs)
 - #67629 (Remove redundant link texts)
 - #67632 (Convert collapsed to shortcut reference links)
 - #67633 (Update .mailmap)
 - #67635 (Document safety of Path casting)
 - #67654 (Add regression test for old NLL ICE)
 - #67659 (Stabilize the `matches!` macro)
 - #67664 (Fix some mailmap entries)

Failed merges:

r? @ghost
@bors bors merged commit 2c796ee into rust-lang:master Dec 28, 2019
4 checks passed
4 checks passed
pr Build #20191226.40 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.