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

Added faster intersection with benchmark #10

Closed
wants to merge 2 commits into from

Conversation

peter-scholtens
Copy link
Contributor

The languages can be found faster with binary sort in the list of available languages if we assume that they are sorted in alphabetical order. With 200+ languages this improves intersection with 25%.

src/lib.rs Outdated
///
/// let common_languages = intersection_fast("en-US, en-GB;q=0.5", &["de", "en-GB", "en-US"]);
/// ```
pub fn intersection_fast(raw_languages: &str, supported_languages: &[&str]) -> Vec<String> {
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if intersection_ordered might be a better name? That's the important part, and I think adding ordered to the name would cause people to pay more attention to the docs than fast would

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, better name it after the method than the result. Also the quality member in the struct could be compacted to a f32 instead of a f64, don't you think?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, that's probably fine!

… can be found faster if a binary sorted list of available languages is applied. A small testbench is added with 200+ languages to demonstrate the improvement. Also float quality is reduced from f64 to f32, as practically only 1 or 2 decimal digits are used.
@peter-scholtens
Copy link
Contributor Author

As the CI tests fails on features only available in rust nightly, the cargo bench tests, would it be acceptable to add a detection via this crate? As suggested here.

@mike-engel
Copy link
Owner

@peter-scholtens sorry for the delay, yes I think that's fine

@peter-scholtens
Copy link
Contributor Author

I solved the problem by moving the benches to a special benches directory. No need to import another crate. See solution as provided in #12.

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.

2 participants