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

str::find(char) is slower than it ought ot be #46693

Closed
mystor opened this issue Dec 12, 2017 · 9 comments · Fixed by #46735
Closed

str::find(char) is slower than it ought ot be #46693

mystor opened this issue Dec 12, 2017 · 9 comments · Fixed by #46735
Assignees
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@mystor
Copy link
Contributor

mystor commented Dec 12, 2017

If you try to find a specific character in a string, it performs significantly worse than the theoretical optimal implementation built on memchr.

For example, consider the following benchmark. On my laptop the find method runs at about 90ns/iter, and the memchr method runs at 4ns/iter. I would imagine that we should optimize them such that they are the same.

#![feature(test)]
extern crate memchr;
extern crate test;

use test::{black_box, Bencher};

const DEMO_STRING: &str = "this is a string with a decent number of ascii characters and \n there is a new line in the middle which it should find";

#[bench]
fn bench_find(b: &mut Bencher) {
    b.iter(|| {
        let s = test::black_box(DEMO_STRING);
        s.find('\n')
    });
}

#[bench]
fn bench_memchr(b: &mut Bencher) {
    b.iter(|| {
        let s = test::black_box(DEMO_STRING);
        memchr::memchr(b'\n', s.as_bytes())
    });
}
@ExpHP
Copy link
Contributor

ExpHP commented Dec 12, 2017

Another benchmark, the intent being to eliminate just the cost of the unnecessary utf8 decoding (that I assume is happening). This gives a meager 60% reduction in time.

#[bench]
fn bench_byte_find(b: &mut Bencher) {
    b.iter(|| {
        let s = test::black_box(DEMO_STRING);
        s.bytes().position(|b| b == b'\n')
    });
}
running 3 tests
test bench_byte_find ... bench:          35 ns/iter (+/- 2)
test bench_find      ... bench:          88 ns/iter (+/- 2)
test bench_memchr    ... bench:           5 ns/iter (+/- 0)

@Manishearth Manishearth self-assigned this Dec 12, 2017
@Manishearth
Copy link
Member

I have fixes. We have two problems.

One is that [u8]::contains doesn't use memchr, which affects str::contains.

The other is that we reuse the same Searcher code for char, [char], and Fn(char) -> bool; we should have a simpler custom impl for char.

@Manishearth
Copy link
Member

#46713 fixes [u8]::contains but not the rest (however it sets the stage for this fix)

@ExpHP
Copy link
Contributor

ExpHP commented Dec 13, 2017

I wonder if similar concerns apply for &str patterns. From the way UTF-8 is designed, I expect that a dumb non-unicode-aware strstr-like implementation is in fact a valid way to implement &str patterns.

(and then as a bonus, we can get non-ASCII char patterns by encoding the char to &str)

@Manishearth
Copy link
Member

Yes, this is true. I need to think about that more.

There is a question of how useful that will be -- for most multibyte utf8 text the first byte is quite common. But it should still be faster to do the dumb memchr-based thing instead of actually iterating chars.

@ExpHP
Copy link
Contributor

ExpHP commented Dec 13, 2017

Comparison to GNU libc strstr, x86-64

Of course, not 100% apples to oranges since C strings are null-terminated

#![feature(test)]
extern crate libc;
extern crate test;

use libc::c_char;
use std::ffi::{CString};
use test::{black_box, Bencher};

const NEEDLE: &str = "there";
const HAYSTACK: &str = "this is a string with a decent number of ascii characters and \n there is a new line in the middle which it should find";

fn strstr(haystack: *const c_char, needle: *const c_char) -> Option<usize> {
    let found = unsafe { libc::strstr(haystack, needle) };

    if found.is_null() { None }
    else { Some(found as usize - haystack as usize) }
}

#[bench]
fn bench_find(b: &mut Bencher) {
    b.iter(|| {
        let haystack = test::black_box(HAYSTACK);
        let needle = test::black_box(NEEDLE);
        haystack.find(needle)
    });
}

#[bench]
fn bench_strstr(b: &mut Bencher) {
    let haystack = CString::new(HAYSTACK).unwrap();
    let haystack = haystack.as_ptr();
    let needle = CString::new(NEEDLE).unwrap();
    let needle = needle.as_ptr();
    b.iter(|| {
        let haystack = test::black_box(haystack);
        let needle = test::black_box(needle);
        strstr(haystack, needle)
    });
}
test bench_find   ... bench:          56 ns/iter (+/- 1)
test bench_strstr ... bench:          19 ns/iter (+/- 0)

@kennytm kennytm added C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 13, 2017
@kennytm
Copy link
Member

kennytm commented Dec 13, 2017

Duplicate of #41993?

@mystor
Copy link
Contributor Author

mystor commented Dec 13, 2017

Duplicate of #41993?

I think this is probably related, but not exactly a duplicate. The code s.find("\n") is faster than s.find('\n'), but both are still significantly slower than memchr.

@Manishearth
Copy link
Member

#46735

bors added a commit that referenced this issue Dec 21, 2017
Use memchr for str::find(char)

This is a 10x improvement for searching for characters.

This also contains the patches from #46713 . Feel free to land both separately or together.

cc @mystor @alexcrichton

r? @bluss

fixes #46693
bors added a commit that referenced this issue Jan 1, 2018
Use memchr for str::find(char)

This is a 10x improvement for searching for characters.

This also contains the patches from #46713 . Feel free to land both separately or together.

cc @mystor @alexcrichton

r? @bluss

fixes #46693
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. I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants