Skip to content
This repository has been archived by the owner on Oct 23, 2022. It is now read-only.

Use hash_hasher in hashmaps/sets where the key is a Cid/Multihash #258 #467

Merged
merged 5 commits into from
Aug 4, 2021

Conversation

CHr15F0x
Copy link
Contributor

@CHr15F0x CHr15F0x commented Aug 2, 2021

This PR implements a fix for #258, ie. using hash_hasher to improve performance in HashMaps/Sets what utilize Cid as key. hash_hasher provides approx. 40% increase in performance in this case.

There are two things that still need to be done:

  • code is not linted yet, since this will be done in a separate PR by Mirko,
  • a simple benchmark is included for the sake of comparing HashedMap<Cid,> vs HashMap<Cid,>; Imho this bench should not be merged.

@koivunej koivunej linked an issue Aug 2, 2021 that may be closed by this pull request
Copy link
Collaborator

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

Looking good!

src/subscription.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@CHr15F0x CHr15F0x force-pushed the chris-258-use-hash-hasher branch from e6204ae to 1e92392 Compare August 2, 2021 17:48
@CHr15F0x
Copy link
Contributor Author

CHr15F0x commented Aug 4, 2021

Comparison using fetch_and_cat on a 10GiB file shows some slight improvement:

Benchmark #1: ./opt-fetch_and_cat QmTR3tCt1SRd96t7rSZpvMPbj48GijPz8crEuuujSygEC1 /ip4/127.0.0.1/tcp/33109/p2p/QmTjjZBvf9fZxUcSqqdXPy135zWwCisGpswAF38jvQ8Y9G > /dev/null
  Time (mean ± σ):     58.706 s ±  0.407 s    [User: 46.545 s, System: 24.116 s]
  Range (min … max):   57.808 s … 59.624 s    100 runs

Benchmark #1: ./fetch_and_cat     QmTR3tCt1SRd96t7rSZpvMPbj48GijPz8crEuuujSygEC1 /ip4/127.0.0.1/tcp/33109/p2p/QmTjjZBvf9fZxUcSqqdXPy135zWwCisGpswAF38jvQ8Y9G > /dev/null
  Time (mean ± σ):     58.838 s ±  0.425 s    [User: 46.751 s, System: 24.169 s]
  Range (min … max):   57.927 s … 59.728 s    100 runs

@koivunej
Copy link
Collaborator

koivunej commented Aug 4, 2021

This needs to have the Cargo.lock removed, sorry for not seeing that. After that, it's good to go. Thanks for the benchmarking.

bors d+

@bors
Copy link
Contributor

bors bot commented Aug 4, 2021

✌️ CHr15F0x can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@koivunej
Copy link
Collaborator

koivunej commented Aug 4, 2021

bors d-

Could you add a CHANGELOG.md entry?

@CHr15F0x
Copy link
Contributor Author

CHr15F0x commented Aug 4, 2021

Changelog is there, let's try with Mr b

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 4, 2021

Merge conflict.

@CHr15F0x
Copy link
Contributor Author

CHr15F0x commented Aug 4, 2021

bors r+

bors bot added a commit that referenced this pull request Aug 4, 2021
467: Use hash_hasher in hashmaps/sets where the key is a Cid/Multihash #258 r=CHr15F0x a=CHr15F0x

This PR implements a fix for #258, ie. using hash_hasher to improve performance in HashMaps/Sets what utilize Cid as key. hash_hasher provides approx. 40% increase in performance in this case.

There are two things that still need to be done:
- code is **not linted** yet, since this will be done in a separate PR by Mirko,
- a simple benchmark is included for the sake of comparing HashedMap<Cid,> vs HashMap<Cid,>; Imho this bench should not be merged.


Co-authored-by: Krzysztof Lis <[email protected]>
Co-authored-by: CHr15F0x <[email protected]>
@koivunej
Copy link
Collaborator

koivunej commented Aug 4, 2021

bors r-

@bors
Copy link
Contributor

bors bot commented Aug 4, 2021

Canceled.

@koivunej
Copy link
Collaborator

koivunej commented Aug 4, 2021

@CHr15F0x could you avoid the merge commit, rebase on master instead?

@CHr15F0x CHr15F0x force-pushed the chris-258-use-hash-hasher branch from 6bc3f31 to f6677d0 Compare August 4, 2021 13:02
@CHr15F0x
Copy link
Contributor Author

CHr15F0x commented Aug 4, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 4, 2021

Build succeeded:

@bors bors bot merged commit b37811f into rs-ipfs:master Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use hash_hasher in hashmaps/sets where the key is a Cid/Multihash
2 participants