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

core::slice::Iter and core::slice::IterMut aren't NonNull-optimized #67228

Closed
Kixunil opened this issue Dec 11, 2019 · 3 comments · Fixed by #67588
Closed

core::slice::Iter and core::slice::IterMut aren't NonNull-optimized #67228

Kixunil opened this issue Dec 11, 2019 · 3 comments · Fixed by #67588
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. I-heavy Issue: Problems and improvements with respect to binary size of generated code. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Kixunil
Copy link
Contributor

Kixunil commented Dec 11, 2019

I randomly noticed that self.ptr of core::slice::Iter may never be null (also because of unconditionally calling from_raw_parts), but it's not contained in NonNull<T>, so it isn't optimized.

I double checked for compiler internal Voodoo and it confirms the lack of optimization.

I was thinking about fixing it just for fun, but it occurred to me that the current code uses assume at several places and maybe it'd be a good idea to rather add assume into NonNull::as_ptr to clean it up? Not sure, so I'm asking. (Also maybe add assume to <[T]>::as_ptr(), but that's a different topic.)

@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. I-heavy Issue: Problems and improvements with respect to binary size of generated code. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 11, 2019
@Mark-Simulacrum
Copy link
Member

I suspect that directly using NonNull here is much better than an assume inside NonNull::as_ptr; assume often has poor interactions with LLVM's optimizer (especially when frequently inserted, as it would be with e.g., NonNull::as_ptr).

I would personally accept a PR that fixes this assuming that the code is improved by such a change and benchmarks do not regress.

@Kixunil
Copy link
Contributor Author

Kixunil commented Dec 23, 2019

I'm highly surprised assume has poor interactions, I'd expect it to be noop at worst. Sounds like an LLVM bug?

Anyway, I guess I'd go with minimal PR that doesn't mess with it.

@Mark-Simulacrum
Copy link
Member

assume can "overfit" by providing too much information in ways that LLVM is not quite prepared for essentially. The minimal PR is likely better in any case :)

JohnTitor added a commit to JohnTitor/rust that referenced this issue 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 bors closed this as completed in 4b91966 Dec 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. I-heavy Issue: Problems and improvements with respect to binary size of generated code. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants