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

*: use fxhash #48

Merged
merged 2 commits into from
Apr 14, 2018
Merged

*: use fxhash #48

merged 2 commits into from
Apr 14, 2018

Conversation

BusyJay
Copy link
Member

@BusyJay BusyJay commented Apr 3, 2018

FlatMap sorts keys, which is unnecessary for small groups. A Raft group usually has 3 ~ 5 nodes.

@siddontang
Copy link
Contributor

can you give a benchmark with them?

@BusyJay
Copy link
Member Author

BusyJay commented Apr 3, 2018

Simple benchmark for querying on 6 nodes.

test bench_default_hashmap ... bench:          55 ns/iter (+/- 33)
test bench_flatmap_hashmap ... bench:          11 ns/iter (+/- 0)
test bench_fnv_hashmap     ... bench:           3 ns/iter (+/- 0)
test bench_fxhash_hashmap  ... bench:           4 ns/iter (+/- 5)
test bench_number_hashmap  ... bench:           4 ns/iter (+/- 3)
test bench_vecmap_hashmap  ... bench:           3 ns/iter (+/- 3)

Another benchmark by the crate author.

@siddontang
Copy link
Contributor

how about 3 nodes?

@BusyJay
Copy link
Member Author

BusyJay commented Apr 3, 2018

I don't think there will be much difference as it only cost 4ns already. We don't just use it for progresses but also for read index.

log = "0.3"
protobuf = "1.2"
quick-error = "0.2"
rand = "0.4"
fxhash = "0.2.1"
Copy link
Contributor

@Hoverbear Hoverbear Apr 3, 2018

Choose a reason for hiding this comment

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

Hm, this library has has an open PR since December that hasn't been addressed: cbreeden/fxhash#3. It does appear to be used in Firefox though. The code seems good overall. I am kind of sad to move away from servo maintained libraries like fnv though.

@siddontang
Copy link
Contributor

LGTM

PTAL @Hoverbear

@siddontang siddontang closed this Apr 4, 2018
@siddontang siddontang reopened this Apr 4, 2018
@BusyJay
Copy link
Member Author

BusyJay commented Apr 9, 2018

PTAL

@Hoverbear
Copy link
Contributor

My primary concern in this case is just that fxhash doesn't seem to be seriously maintained, while fnv does.

@siddontang
Copy link
Contributor

I find that servo also uses it, so I think it is fine.

@Hoverbear
Copy link
Contributor

Then let's do it. :)

@siddontang siddontang merged commit 288db2e into master Apr 14, 2018
@siddontang siddontang deleted the use-fxhash branch April 14, 2018 01:47
@Hoverbear Hoverbear added this to the 0.2.0 milestone Jul 5, 2018
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