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

Specialization for StepBy<ops::Range<T>> #108615

Closed
dhulke opened this issue Mar 1, 2023 · 6 comments
Closed

Specialization for StepBy<ops::Range<T>> #108615

dhulke opened this issue Mar 1, 2023 · 6 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@dhulke
Copy link

dhulke commented Mar 1, 2023

Evidence

I remember seeing this commit that specialized StepBy for ops::Range and today when looking back at the library's source code I saw that the specialization is gone.
Here's the specialization commit merged: 95979dc
Here's the current code for StepBy::next(): https://github.com/rust-lang/rust/blob/master/library/core/src/iter/adapters/step_by.rs#L34

Why was this specialization removed and can we add it back?

@dhulke dhulke added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Mar 1, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 1, 2023
@Noratrieb
Copy link
Member

Noratrieb commented Mar 1, 2023

The PR was immediately reverted in #56049 because it was broken. It may be able to be readded if someone finds a way to fix it.

@Noratrieb Noratrieb added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed regression-untriaged Untriaged performance or correctness regression. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 1, 2023
@jyn514
Copy link
Member

jyn514 commented Mar 1, 2023

I don't think this should be treated as a regression.

@jyn514 jyn514 added C-enhancement Category: An issue proposing an enhancement or a PR with one. and removed C-bug Category: This is a bug. labels Mar 1, 2023
@dhulke
Copy link
Author

dhulke commented Mar 1, 2023

@Nilstrieb Thank you, I missed that revert. I might pick this up in the next couple of weeks then. Should we close the issue and I open a new discussion with a PR pointing to this issue in the future?

@Noratrieb
Copy link
Member

Keeping it open is fine, it's a suggestion for an improvement to the standard library

@the8472
Copy link
Member

the8472 commented Mar 1, 2023

This would be a welcome improvement, e.g. in #103779 I had to handroll loops because range + step_by wasn't up to snuff.

while i + last_byte_offset + Block::LANES < haystack.len() && !result {
let mask = test_chunk(i);
if mask != 0 {
result |= check_mask(i, mask, result);
}
i += Block::LANES;
}

@the8472
Copy link
Member

the8472 commented May 20, 2023

Closing as duplicate of #51557

@the8472 the8472 closed this as completed May 20, 2023
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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants