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

Improve code generated for `starts_with(<literal char>)` #67249

Merged
merged 4 commits into from Dec 16, 2019

Conversation

@ranma42
Copy link
Contributor

ranma42 commented Dec 12, 2019

This PR includes two minor improvements to the code generated when checking for string prefix/suffix.

The first commit simplifies the str/str operation, by taking advantage of the raw UTF-8 representation.

The second commit replaces the current str/char matching logic with a char->str encoding and then the previous method.

The resulting code should be equivalent in the generic case (one char is being encoded versus one char being decoded), but it becomes easy to optimize in the case of a literal char, which in most cases a developer might expect to be at least as simple as that of a literal string.

This PR should fix #41993

ranma42 added 2 commits Dec 11, 2019
The comparison can be performed on the raw bytes, as the chars can
only match if their UTF8 encoding matches.

This avoids the `is_char_boundary` checks and translates to a straight
`u8` slice comparison which is optimized to a memcmp or inline
comparison where appropriate.
This enables constant folding when matching a literal char.

Fixes #41993.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 12, 2019

r? @shepmaster

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

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Dec 12, 2019

cc @kennytm

r? @BurntSushi perhaps?

Copy link
Member

BurntSushi left a comment

The change itself LGTM. Do you have benchmarks showing a difference here? Mostly just to confirm there aren't any regressions. Even with no benefit, I think these changes make the code simpler.

@@ -715,16 +715,13 @@ impl<'a, 'b> Pattern<'a> for &'b str {
/// Checks whether the pattern matches at the front of the haystack
#[inline]
fn is_prefix_of(self, haystack: &'a str) -> bool {
haystack.is_char_boundary(self.len()) &&

This comment has been minimized.

Copy link
@BurntSushi

BurntSushi Dec 12, 2019

Member

This is interesting. According to git blame, @bluss added this in 2015. But yeah, this looks unnecessary to me and I agree with the change.

false
}
let mut buffer = [0u8; 4];
self.encode_utf8(&mut buffer).is_prefix_of(haystack)

This comment has been minimized.

Copy link
@BurntSushi

BurntSushi Dec 12, 2019

Member

I think this can just be simplified to self.encode_utf8(&mut [0; 4]).is_prefix_of(haystack)? And similarly for below.

This comment has been minimized.

Copy link
@ranma42

ranma42 Dec 12, 2019

Author Contributor

You are right; I added a cleanup commit.

@ranma42

This comment has been minimized.

Copy link
Contributor Author

ranma42 commented Dec 12, 2019

I used this code to check the generated assembly.
I benchmarked against the current nightly:

$ rustc --version
rustc 1.41.0-nightly (27d6f55f4 2019-12-11)
$ rustc -C opt-level=3 --test starts_with.rs
$ ./starts_with --bench

running 4 tests
test bench_ends_with_char     ... bench:         437 ns/iter (+/- 21)
test bench_ends_with_string   ... bench:       1,405 ns/iter (+/- 86)
test bench_starts_with_char   ... bench:         713 ns/iter (+/- 19)
test bench_starts_with_string ... bench:         995 ns/iter (+/- 37)

test result: ok. 0 passed; 0 failed; 0 ignored; 4 measured; 0 filtered out

$ ./rust/build/x86_64-apple-darwin/stage2/bin/rustc --version
rustc 1.41.0-dev
$ ./rust/build/x86_64-apple-darwin/stage2/bin/rustc -C opt-level=3 --test starts_with.rs
$ ./starts_with --bench

running 4 tests
test bench_ends_with_char     ... bench:         395 ns/iter (+/- 13)
test bench_ends_with_string   ... bench:         314 ns/iter (+/- 29)
test bench_starts_with_char   ... bench:         315 ns/iter (+/- 12)
test bench_starts_with_string ... bench:         315 ns/iter (+/- 24)

test result: ok. 0 passed; 0 failed; 0 ignored; 4 measured; 0 filtered out
@BurntSushi

This comment has been minimized.

Copy link
Member

BurntSushi commented Dec 13, 2019

@ranma42 Is there a place where those benchmarks can be added in this PR?

@ranma42

This comment has been minimized.

Copy link
Contributor Author

ranma42 commented Dec 16, 2019

I added the benchmarks in 3de1923 , but I am not completely sure if that is the correct/best way to do it.

@BurntSushi

This comment has been minimized.

Copy link
Member

BurntSushi commented Dec 16, 2019

LGTM. Thanks so much!

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 16, 2019

📌 Commit 3de1923 has been approved by BurntSushi

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 16, 2019

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

Centril added a commit to Centril/rust that referenced this pull request Dec 16, 2019
…-char, r=BurntSushi

Improve code generated for `starts_with(<literal char>)`

This PR includes two minor improvements to the code generated when checking for string prefix/suffix.

The first commit simplifies the str/str operation, by taking advantage of the raw UTF-8 representation.

The second commit replaces the current str/char matching logic with a char->str encoding and then the previous method.

The resulting code should be equivalent in the generic case (one char is being encoded versus one char being decoded), but it becomes easy to optimize in the case of a literal char, which in most cases a developer might expect to be at least as simple as that of a literal string.

This PR should fix rust-lang#41993
Centril added a commit to Centril/rust that referenced this pull request Dec 16, 2019
…-char, r=BurntSushi

Improve code generated for `starts_with(<literal char>)`

This PR includes two minor improvements to the code generated when checking for string prefix/suffix.

The first commit simplifies the str/str operation, by taking advantage of the raw UTF-8 representation.

The second commit replaces the current str/char matching logic with a char->str encoding and then the previous method.

The resulting code should be equivalent in the generic case (one char is being encoded versus one char being decoded), but it becomes easy to optimize in the case of a literal char, which in most cases a developer might expect to be at least as simple as that of a literal string.

This PR should fix rust-lang#41993
bors added a commit that referenced this pull request Dec 16, 2019
Rollup of 8 pull requests

Successful merges:

 - #67249 (Improve code generated for `starts_with(<literal char>)`)
 - #67308 (Delete flaky test net::tcp::tests::fast_rebind)
 - #67318 (Improve typeck & lowering docs for slice patterns)
 - #67322 (use Self alias in place of macros)
 - #67323 (make transparent enums more ordinary)
 - #67336 (Fix JS error when loading page with search)
 - #67344 (.gitignore: Don't ignore a file that exists in the repository)
 - #67349 (Minor: update Unsize docs for dyn syntax)

Failed merges:

r? @ghost
@bors bors merged commit 3de1923 into rust-lang:master Dec 16, 2019
4 checks passed
4 checks passed
pr #20191216.29 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.