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.

Sign up for GitHub

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

Jump to bottom

Use the slice length to hint the optimizer about iter.position result #47772

Merged
merged 1 commit into from
Jan 28, 2018
Merged

Use the slice length to hint the optimizer about iter.position result #47772

merged 1 commit into from
Jan 28, 2018

Conversation

arthurprs
Copy link
Contributor

@arthurprs arthurprs commented Jan 26, 2018 •
edited
Loading

Using the len of the iterator doesn't give the same result.
That's also why we can't generalize it to all TrustedLen iterators.

Problem demo: https://godbolt.org/g/MXg2ae
Fix demo: https://godbolt.org/g/P8q5aZ

Second attempt of #47333
Third attempt of #45501
Fixes #45964

@rust-highfive
Copy link
Collaborator

r? @cramertj

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

@arthurprs
Use the slice length to hint the optimizer
4f7109a 
Using the len of the iterator doesn't give the same result.
That's also why we can't generalize it to all TrustedLen iterators.
@arthurprs arthurprs force-pushed the iter-position-bounds-check branch from 40cb096 to 4f7109a Compare January 26, 2018 11:49
@arthurprs
Copy link
Contributor Author

arthurprs commented Jan 26, 2018 •
edited
Loading

I suspect it is because ptr::offset_to (iter size_hint/len) uses signed arithmetic whereas make_slice uses unsigned. Demo: https://godbolt.org/g/avqu9P

Edit: It might be better to change the usage of offset_to in this module. Thoughts?

@kennytm kennytm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 26, 2018
@cramertj
Copy link
Member

r? @dtolnay

@rust-highfive rust-highfive assigned dtolnay and unassigned cramertj Jan 26, 2018
dtolnay
dtolnay approved these changes Jan 26, 2018
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks like this should finally do the trick. Let's go with this and follow up on the other uses of offset_of in this module if we notice further performance problems.

@dtolnay
Copy link
Member

dtolnay commented Jan 26, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Jan 26, 2018

📌 Commit 4f7109a has been approved by dtolnay

@bors
Copy link
Contributor

bors commented Jan 28, 2018

⌛ Testing commit 4f7109a with merge 0119b44...

bors added a commit that referenced this pull request Jan 28, 2018
@bors
Auto merge of #47772 - arthurprs:iter-position-bounds-check, r=dtolnay
0119b44 
Use the slice length to hint the optimizer about iter.position result

Using the len of the iterator doesn't give the same result.
That's also why we can't generalize it to all TrustedLen iterators.

Problem demo: https://godbolt.org/g/MXg2ae
Fix demo: https://godbolt.org/g/P8q5aZ

Second attempt of #47333
Third attempt of #45501
Fixes #45964
@bors
Copy link
Contributor

bors commented Jan 28, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: dtolnay
Pushing 0119b44 to master...

@bors bors merged commit 4f7109a into rust-lang:master Jan 28, 2018
@RalfJung RalfJung mentioned this pull request Jul 28, 2018
Merged
RalfJung added a commit to RalfJung/rust that referenced this pull request Jul 29, 2018
@RalfJung
Make sure rust-lang#47772 does not regress
90165b8
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jul 30, 2018
Closed
RalfJung added a commit to RalfJung/rust that referenced this pull request Jul 30, 2018
@RalfJung
Make sure rust-lang#47772 does not regress
f34650a
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 30, 2018
@Mark-Simulacrum
Rollup merge of rust-lang#52825 - RalfJung:codegen, r=alexcrichton
5739542 
Make sure rust-lang#47772 does not regress

Mostly to make my life in rust-lang#52206 harder.^^

Or should I just add that test there?
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jul 30, 2018
Closed
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 30, 2018
@Mark-Simulacrum
Rollup merge of rust-lang#52825 - RalfJung:codegen, r=alexcrichton
0fa7c12 
Make sure rust-lang#47772 does not regress

Mostly to make my life in rust-lang#52206 harder.^^

Or should I just add that test there?
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jul 30, 2018
Closed
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Aug 1, 2018
@pietroalbini
Rollup merge of rust-lang#52825 - RalfJung:codegen, r=alexcrichton
fe4358a 
Make sure rust-lang#47772 does not regress

Mostly to make my life in rust-lang#52206 harder.^^

Or should I just add that test there?
@pietroalbini pietroalbini mentioned this pull request Aug 1, 2018
Merged
bors added a commit that referenced this pull request Aug 1, 2018
@bors
Auto merge of #52937 - pietroalbini:rollup, r=pietroalbini
8c069ce 
Rollup of 30 pull requests

Successful merges:

 - #52340 (Document From trait implementations for OsStr, OsString, CString, and CStr)
 - #52628 (Cleanup some rustdoc code)
 - #52732 (Remove unstable and deprecated APIs)
 - #52745 (Update clippy to latest master)
 - #52771 (Clarify thread::park semantics)
 - #52778 (Improve readability of serialize.rs)
 - #52810 ([NLL] Don't make "fake" match variables mutable)
 - #52821 (pretty print for std::collections::vecdeque)
 - #52822 (Fix From<LocalWaker>)
 - #52824 (Fix -Wpessimizing-move warnings in rustllvm/PassWrapper)
 - #52825 (Make sure #47772 does not regress)
 - #52831 (remove references to AUTHORS.txt file)
 - #52842 (update comment)
 - #52846 (Add timeout to use of `curl` in bootstrap.py.)
 - #52851 (Make the tool_lints actually usable)
 - #52853 (Improve bootstrap help on stages)
 - #52859 (Use Vec::extend in SmallVec::extend when applicable)
 - #52861 (Add targets for HermitCore (https://hermitcore.org) to the Rust compiler and port libstd to it.)
 - #52867 (releases.md: fix 2 typos)
 - #52870 (Implement Unpin for FutureObj and LocalFutureObj)
 - #52876 (run-pass/const-endianness: negate before to_le())
 - #52878 (Fix wrong issue number in the test name)
 - #52883 (Include lifetime in mutability suggestion in NLL messages)
 - #52888 (Use suggestions for shell format arguments)
 - #52904 (NLL: sort diagnostics by span)
 - #52905 (Fix a typo in unsize.rs)
 - #52907 (NLL: On "cannot move out of type" error, print original before rewrite)
 - #52914 (Only run the sparc-abi test on sparc)
 - #52918 (Backport 1.27.2 release notes)
 - #52929 (Update compatibility note for 1.28.0 to be correct)

Failed merges:

r? @ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@arthurprs @rust-highfive @cramertj @dtolnay @bors @kennytm