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

Allow the use of ahash as the default hasher for more then redis. #171

Closed
wants to merge 0 commits into from

Conversation

FilipAndersson245
Copy link
Contributor

@FilipAndersson245 FilipAndersson245 commented Sep 10, 2023

This PR adds a new feature ahash that changes primarily what hashing algorithm is used to hash data, instead opting for the faster ahash algorithm. It also changes the hashtable implementation instead to be directly depending on hashbrown for sized we already depended directly on hashbrown so there the only change is the ahash hashing compared to the default. The default in hashbrown is to use ahash so for the other ones I opted just to use hashbrown directly. Maybe this could even be simplified as we could use hashbrown everywhere.

✅cargo fmt
✅update changelog

@FilipAndersson245
Copy link
Contributor Author

I did a very basic performance comparason using the fib example, using hyperfine

❯ hyperfine --warmup 50 --runs 1000 --shell none './fib' './fib-ahash'
Benchmark 1: ./fib
  Time (mean ± σ):       5.6 ms ±   1.1 ms    [User: 4.5 ms, System: 1.1 ms]
  Range (min … max):     4.5 ms …  13.1 ms    1000 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Benchmark 2: ./fib-ahash
  Time (mean ± σ):       5.2 ms ±   1.2 ms    [User: 4.0 ms, System: 1.2 ms]
  Range (min … max):     3.8 ms …   9.3 ms    1000 runs

Summary
  ./fib-ahash ran
    1.08 ± 0.33 times faster than ./fib

@FilipAndersson245
Copy link
Contributor Author

@jaemk anything else that has to be done to merge this PR?

src/stores/timed.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated
@@ -18,6 +18,7 @@ rustdoc-args = ["--cfg", "docsrs"]
[features]
default = ["proc_macro"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it should be the default hasher or not. @jaemk ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

imho, I see little downsides using this as default.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@jaemk
Copy link
Owner

jaemk commented Sep 21, 2023

@FilipAndersson245 sorry for the delay, thank you for adding this! I'll merge and release later today

@FilipAndersson245
Copy link
Contributor Author

FilipAndersson245 commented Sep 21, 2023

@FilipAndersson245 sorry for the delay, thank you for adding this! I'll merge and release later today

no problem 😄
What would be your thoughts on making this part of the default? it would not increase compile time as its part of hashbrowns already and has some neat perf improvement.

@jaemk
Copy link
Owner

jaemk commented Sep 21, 2023

I'm on board with making it the default hash algo. Can you also update the crate-level docs explaining that it's the default

@FilipAndersson245
Copy link
Contributor Author

FilipAndersson245 commented Sep 21, 2023

I'm on board with making it the default hash algo. Can you also update the crate-level docs explaining that it's the default

done

@jaemk
Copy link
Owner

jaemk commented Sep 22, 2023

@FilipAndersson245 would you mind running make docs and pulling in changes from master

@FilipAndersson245
Copy link
Contributor Author

Hello had to do some rebasing of the commits everything should be fine now I believe.

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.

3 participants