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

Remove hashbrown from trie cache. #2632

Merged
merged 3 commits into from
Dec 8, 2023
Merged

Conversation

cheme
Copy link
Contributor

@cheme cheme commented Dec 5, 2023

Using hashmap instead (hashset do not expose entry), to get the default random hasher her.

@cheme cheme added the T0-node This PR/Issue is related to the topic “node”. label Dec 5, 2023
@bkchr bkchr requested a review from koute December 5, 2023 22:01
@bkchr bkchr added R0-silent Changes should not be mentioned in any release notes and removed T0-node This PR/Issue is related to the topic “node”. labels Dec 5, 2023
@bkchr bkchr enabled auto-merge (squash) December 5, 2023 22:02
@koute
Copy link
Contributor

koute commented Dec 6, 2023

Can you explain why are we changing this? Is it because the hash map from hashbrown by default only generates a completely new seed once at startup? In which case we could probably just do:

use ahash::AHashSet as HashSet;

instead, which would still use ahash (which is going to be faster than the default hash in Rust's std), but it will use its RandomState which will always give a unique seed to each hash map/hash set.

Here's an example program to demonstrate:

use core::hash::BuildHasher;
use core::hash::Hasher;

fn main() {
    println!("ahash::AHashSet");
    for _ in 0..2 {
        let mut h = ahash::AHashSet::<i32>::new();
        let mut hs = h.hasher();
        let mut hh = hs.build_hasher();
        hh.write_u32(1234);
        println!("  {:?}", hh.finish());
    }

    println!("std::collections::HashSet");
    for _ in 0..2 {
        let mut h = std::collections::HashSet::<i32>::new();
        let mut hs = h.hasher();
        let mut hh = hs.build_hasher();
        hh.write_u32(1234);
        println!("  {:?}", hh.finish());
    }

    println!("hashbrown::HashSet");
    for _ in 0..2 {
        let mut h = hashbrown::HashSet::<i32>::new();
        let mut hs = h.hasher();
        let mut hh = hs.build_hasher();
        hh.write_u32(1234);
        println!("  {:?}", hh.finish());
    }
}

Result:

ahash::AHashSet
  9713553790381145631
  1617804629700195850
std::collections::HashSet
  12330284507346958173
  15536535817085373979
hashbrown::HashSet
  3076938710444694377
  3076938710444694377

@bkchr
Copy link
Member

bkchr commented Dec 6, 2023

Yeah, I also told @cheme the same. But in general, I don't see any big problem of just removing hashbrown.

@cheme
Copy link
Contributor Author

cheme commented Dec 6, 2023

Yes, it is totally right to use hashbrown. (when I write this I did check hashbrown code on a local fork that was reallly outdated and it was still using comptime_rand, but it is not the case anymore since 2020 :).

I only let the PR to avoid having hashbrown dependency here (one less dep in the Cargo.toml), but it really don't have to be merge, so just to lower deps number.

@koute
Copy link
Contributor

koute commented Dec 6, 2023

I only let the PR to avoid having hashbrown dependency here (one less dep in the Cargo.toml), but it really don't have to be merge, so just to lower deps number.

Well, ultimately even if you do remove the hashbrown dependency it won't actually get removed, because schnellru will always depend on hashbrown, so we'll still be using it transitively anyway.

Anyway, you can remove hashbrown if you want (as the HashSet in std is hashbrown anyway); I'd just like to keep the faster hasher from ahash as-is, since this part of the code is speed sensitive.

@cheme
Copy link
Contributor Author

cheme commented Dec 6, 2023

Of course, this should not change the actual generated code (only make sense so user/reader don't ask themselve why we have hashbrown and also for version mgmt even if if we want to strictly check speed at each version update it may make sense to keep hashbrown).

@bkchr
Copy link
Member

bkchr commented Dec 6, 2023

I'd just like to keep the faster hasher from ahash as-is, since this part of the code is speed sensitive.

Can you please do this @cheme?

@cheme
Copy link
Contributor Author

cheme commented Dec 6, 2023

I'd just like to keep the faster hasher from ahash as-is, since this part of the code is speed sensitive.

Can you please do this @cheme?

Sure (I kind of miss the info 🙈)

@bkchr
Copy link
Member

bkchr commented Dec 7, 2023

@koute please.

@bkchr bkchr merged commit 34c991e into paritytech:master Dec 8, 2023
115 checks passed
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
Using hashmap instead (hashset do not expose entry), to get the default
random hasher her.
bkontur pushed a commit that referenced this pull request Jun 5, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
bkontur pushed a commit that referenced this pull request Jun 7, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
bkontur pushed a commit that referenced this pull request Jun 7, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
bkontur pushed a commit that referenced this pull request Jun 12, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
bkontur pushed a commit that referenced this pull request Jun 13, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
bkontur pushed a commit that referenced this pull request Jun 14, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
bkontur pushed a commit that referenced this pull request Jun 14, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
bkontur pushed a commit that referenced this pull request Jun 14, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
bkontur pushed a commit that referenced this pull request Jun 14, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
bkontur pushed a commit that referenced this pull request Jun 20, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
bkontur pushed a commit that referenced this pull request Jun 20, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
bkontur pushed a commit that referenced this pull request Jun 26, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
bkontur pushed a commit that referenced this pull request Jun 26, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
bkontur pushed a commit that referenced this pull request Jun 28, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
bkontur pushed a commit that referenced this pull request Jul 1, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
bkontur pushed a commit that referenced this pull request Jul 4, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
bkontur pushed a commit that referenced this pull request Jul 16, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
bkontur pushed a commit that referenced this pull request Jul 17, 2024
* relayer_reward_per_message is now Option<RelayerRewardAtSource>

* fixed benchmarks compilation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants