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

Refactor memchr to allow optimization #76971

Merged
merged 2 commits into from
Oct 1, 2020
Merged

Refactor memchr to allow optimization #76971

merged 2 commits into from
Oct 1, 2020

Conversation

bugadani
Copy link
Contributor

Closes #75659

The implementation already uses naive search if the slice if short enough, but the case is complicated enough to not be optimized away. This PR refactors memchr so that it exists early when the slice is short enough.

Codegen-wise, as shown in #75659, memchr was not inlined previously so the only way I could find to test this is to check if there is no memchr call. Let me know if there is a more robust solution here.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 20, 2020
@joshtriplett
Copy link
Member

I'm not the right reviewer for this. Someone from libs should review.

@bugadani
Copy link
Contributor Author

r? @dtolnay if I may ask for a review

@jyn514 jyn514 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 25, 2020
dtolnay
dtolnay approved these changes Sep 25, 2020
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.

Nice!

@dtolnay
Copy link
Member

dtolnay commented Sep 25, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Sep 25, 2020

📌 Commit 37f08c7 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2020
@jonas-schievink jonas-schievink mentioned this pull request Sep 25, 2020
Closed
@Amanieu
Copy link
Member

Amanieu commented Sep 25, 2020

... I don't see how this test even works since memchr is not #[inline].

Also the test is passing because it is calling slice_contains (which is also not #[inline]), not memchr.

@bors r-

@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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 25, 2020
@bugadani
Copy link
Contributor Author

bugadani commented Sep 25, 2020 •
edited
Loading

Also the test is passing because it is calling slice_contains (which is also not #[inline]), not memchr.

That's fair, although I remember the test fail without the patch. o.O

@Amanieu
Copy link
Member

Amanieu commented Sep 25, 2020

I just checked it locally: your test passes without the patch.

@bugadani
Copy link
Contributor Author

I just checked it locally: your test passes without the patch.

@Amanieu and fails when I check for slice_contains. I'm looking into it, thanks a lot for noticing!

@bugadani bugadani force-pushed the issue-75659 branch 2 times, most recently from adbda0d to ac0b79d Compare September 25, 2020 19:39
@bugadani
Copy link
Contributor Author

bugadani commented Sep 25, 2020 •
edited
Loading

I've figured out what the basis of my assumption was: https://godbolt.org/z/sozbsr I've tried the refactor first on godbolt.org and it produced the expected assembly. I guess the difference comes from the code being in the same crate as the called, so it's inlined without explicit #[inline] hints. I'm currently waiting for test results locally, but Let me know if sprinkling around a ton of #[inline]s is not something I should be doing here.

the8472
the8472 reviewed Sep 25, 2020
View reviewed changes
library/core/src/slice/memchr.rs Outdated
memchr_general_case(x, text)
}

#[inline(never)]
Copy link
Member

Choose a reason for hiding this comment

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

couldn't this inhibit const folding by llvm in some cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least in my godbolt-example, as far as I can tell, after 16 elements the whole memchr code would be inlined instead of folding. So technically, yes, but I'm not sure in practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the other hand, currently there is no const folding here anyway :)

@Amanieu
Copy link
Member

Amanieu commented Sep 26, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Sep 26, 2020

📌 Commit d023436 has been approved by Amanieu

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 26, 2020
@bugadani
Copy link
Contributor Author

I've modified foo8_i8 so that it won't get merged with foo8 again.

@bugadani
Copy link
Contributor Author

@Amanieu I can add the whole path to the test but I don't think it matters. Let me know if you want me to do it regardless.

@Amanieu
Copy link
Member

Amanieu commented Sep 29, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Sep 29, 2020

📌 Commit 89b8a97 has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 29, 2020
@bors
Copy link
Contributor

bors commented Sep 29, 2020

⌛ Testing commit 89b8a97 with merge d10740de8e266e96b0955da0e642a517920e8d20...

@bors
Copy link
Contributor

bors commented Sep 29, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 29, 2020
@bugadani
Copy link
Contributor Author

Looks like this is a lot more brittle than I thought... Oh well. Thanks for everyone's time.

@bugadani bugadani closed this Sep 29, 2020
@bugadani bugadani reopened this Oct 1, 2020
@bugadani
Copy link
Contributor Author

bugadani commented Oct 1, 2020 •
edited
Loading

I've given this one some more thought. It's cool to enable an optimization, but since the array size for which the fast path can apply is architecture-dependent, I think I'm going to only enable the test for x86-64. Since the patch shouldn't break anything, I think this is okay. Let me know if this is OK or how I should proceed.

@Amanieu
Copy link
Member

Amanieu commented Oct 1, 2020

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Oct 1, 2020

📌 Commit de623bf has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 1, 2020
@bors
Copy link
Contributor

bors commented Oct 1, 2020

⌛ Testing commit de623bf with merge 8fe73e8...

@bors
Copy link
Contributor

bors commented Oct 1, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: Amanieu
Pushing 8fe73e8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 1, 2020
@bors bors merged commit 8fe73e8 into rust-lang:master Oct 1, 2020
@rustbot rustbot added this to the 1.48.0 milestone Oct 1, 2020
@bugadani bugadani deleted the issue-75659 branch October 1, 2020 20:33
This was referenced Oct 1, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tesuji tesuji tesuji left review comments

@the8472 the8472 the8472 left review comments

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.

Badly optimized small bytes array search
10 participants
@bugadani @rust-highfive @joshtriplett @dtolnay @bors @Amanieu @the8472 @tesuji @jyn514 @rustbot