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 table search speed through lookups #112

Merged
merged 1 commit into from
Jan 30, 2023

Conversation

indutny
Copy link
Contributor

@indutny indutny commented Jan 28, 2023

Prior to this change table search would have to do a binary search over about 1000 entries which resulted in around 10 memory loads on average. In this commit we reduce the search space by doing a pre-lookup in a generated table to get a smaller (often zero-length) slice of the full sorted range list. On average this gives us just one entry of the range list to perform binary search on, which reduces the average number of memory loads to 2.

@indutny
Copy link
Contributor Author

indutny commented Jan 28, 2023

With the help of #113 this gives:

  • Between 0% and 15% improvement on grapheme benchmark (depending on language)
  • Between 22% and 40% improvement on word benchmarks (depending on language)

Full Report: https://gist.github.com/indutny/9f5f623c52d6e935225edfe09857ef99

@indutny
Copy link
Contributor Author

indutny commented Jan 30, 2023

Rebased over the latest master, but it looks like nothing changed in this PR.

src/tables.rs Outdated
@@ -387,9 +387,81 @@ pub mod grapheme {
}

pub fn grapheme_category(c: char) -> (u32, u32, GraphemeCat) {
bsearch_range_value_table(c, grapheme_cat_table)
let idx = c as usize / 0x80;
Copy link
Member

Choose a reason for hiding this comment

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

please add/generate comments into the code that explains what's going on here

also on the const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! Force pushed with comments!

@indutny indutny force-pushed the feature/lookup-and-search branch 4 times, most recently from f52636c to bb6b82a Compare January 30, 2023 22:04
@indutny-signal
Copy link

Let me run benchmarks on this first, it looks like I might have regressed with my latest refactor.

Prior to this change table search would have to do a binary search over
about 1000 entries which resulted in around 10 memory loads on average.
In this commit we reduce the search space by doing a pre-lookup in a
generated table to get a smaller (often zero-length) slice of the full
sorted range list. On average this gives us just one entry of the range
list to perform binary search on, which reduces the average number of
memory loads to 2.
@Manishearth
Copy link
Member

Alright, let me know when I should look at this!

@indutny
Copy link
Contributor Author

indutny commented Jan 30, 2023

@Manishearth Should be good now, sorry for the back and forth! I made some simplifications and benchmarks looks even better than before: https://gist.github.com/indutny/c8d29a00680bfbaf19d22d02a7175c0d ( with up to 51% improvement on some word bounds benches).

@Manishearth Manishearth merged commit e94d2fe into unicode-rs:master Jan 30, 2023
@Manishearth
Copy link
Member

Thanks!

@indutny indutny deleted the feature/lookup-and-search branch January 30, 2023 23:12
@indutny
Copy link
Contributor Author

indutny commented Jan 30, 2023

Wow! Thank you for merging it!

Not to impose, but I was wondering when you planned to release a new version of the library?

@Manishearth
Copy link
Member

I'd happily merge a PR bumping the library to 1.10.1.

@indutny
Copy link
Contributor Author

indutny commented Jan 30, 2023

You got it!

@jrose-signal
Copy link

jrose-signal commented Jan 30, 2023

Wait, I have one more optimization for you. :-) (EDIT: I'm @indutny's coworker, we've been loosely collaborating and independently looking at optimization opportunities in a few of the unicode-rs crates.)

@jrose-signal
Copy link

Ah, it looks like @indutny's optimization has made mine irrelevant—some texts get faster, others get slower. So it's probably not worth it.

@indutny-signal indutny-signal mentioned this pull request Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants