Slightly faster keyword lookups #1591
Conversation
Its a micro optimization but seemed to give a bit of a boost to only search words starting with the correct letter.
| return Keyword::NoKeyword; | ||
| } | ||
|
|
||
| let word = word.to_uppercase(); |
There was a problem hiding this comment.
if we can figure out how to remove this to_uppercase call I think this approach will likely be very fast.
I dug around in the rust API docs and it seems like we could change the comparison to ignore ascii case. See comment below 🤔
| ALL_KEYWORDS.len() | ||
| }; | ||
|
|
||
| let keyword = ALL_KEYWORDS[start..end].binary_search(&word.as_str()); |
There was a problem hiding this comment.
I wonder if we could use something like
| let keyword = ALL_KEYWORDS[start..end].binary_search(&word.as_str()); | |
| let keyword = ALL_KEYWORDS[start..end].binary_search_by(|s| s.eq_ignore_ascii_case(&word)) |
https://doc.rust-lang.org/std/primitive.slice.html#method.binary_search_by
https://doc.rust-lang.org/std/primitive.str.html#method.eq_ignore_ascii_case
There was a problem hiding this comment.
I actually tried to do this approach, and I found the to_uppercase call is being done as part of the token creation. So to save the allocation we need to to avoid
I think we can change it to make_ascii_uppercase perhaps
| pub const NA: usize = usize::MAX; | ||
|
|
||
| #[rustfmt::skip] | ||
| pub const KEYWORD_LOOKUP_INDEX_ROOT: &[usize; 26] = &[ |
There was a problem hiding this comment.
I think this is not super maintainable (needs to be updated manually when adding keywords).
There was a problem hiding this comment.
I agree - it we go with this type of table driven approach we should have some sort of update script (or build.rs) that builds the table from the enum.
|
Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look |
Its a micro optimization but seemed to give a bit of a boost to only search words starting with the correct letter.
An alternative to #1590 that doesn't involve adding a new dependency. This just relies on having a pre-calculated table of word ranges to binary search.
Relatively speaking, this approach gets about slightly less than half the speed up of using phf.