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

optimize Range[Inclusive].count() #48024

Closed
wants to merge 2 commits into from
Closed

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Feb 5, 2018

This special-cases count() for ranges to reduce them to simple arithmetic.

Open questions:

  • How to deal with 128bit types?
  • What further tests are needed?

@rust-highfive
Copy link
Collaborator

r? @KodrAus

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

@jonas-schievink
Copy link
Contributor

The Iterator::count docs say: "The method does no guarding against overflows, so counting elements of an iterator with more than usize::MAX elements either produces the wrong result or panics. If debug assertions are enabled, a panic is guaranteed."

wrapping_sub won't produce a panic even in debug mode, so perhaps a regular - is better?

@pietroalbini pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 5, 2018
@scottmcm
Copy link
Member

scottmcm commented Feb 6, 2018

Apparently last time this was tried, it hit type inference weirdness: #39975 (comment) Maybe that's what's causing the travis failure?

Also, is this even needed? There's len for when you want the O(1) guarantee, and it seems that LLVM already turns Range::count into a sub in release: https://godbolt.org/g/FGaWje

@bors
Copy link
Contributor

bors commented Feb 6, 2018

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

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 6, 2018
@scottmcm
Copy link
Member

scottmcm commented Feb 7, 2018

Sorry, I should have said that with #48012 (part of the conflicting merge commit above), LLVM will also unloop the RangeInclusive version. Godbolt isn't updated as of writing, but https://play.rust-lang.org/?gist=f0b127c0dd48a1b9971166bd59a6e815&version=nightly&mode=release

(Now, llvm is being a bit dumb here, since it's explicitly branching to return 1 when x == y instead of calculating x-y+1 -- also 1, of course -- but I don't think that's a big deal.)

If we do want to do this, I think it can be done for everything, not just built-ins, by just returning .size_hint().1.expect("overflow"), since that already has the correct logic.

@BatmanAoD
Copy link
Member

@llogiq Ping from triage; is this ready for re-review?

@llogiq llogiq closed this Feb 14, 2018
@llogiq
Copy link
Contributor Author

llogiq commented Feb 14, 2018

I'm going to close this for now, now that #48012 has landed, the wins are negligible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants