feat: add option for using custom hashers

[ndrscodes]

Add custom hash builder support (new_with_hasher)

Adds a generic type parameter S: BuildHasher (defaulting to RandomState) to all cache types.
Each type now exposes a new_with_hasher(...) constructor (and with_shards_and_hasher for sharded variants), allowing callers to plug in a custom hashing algorithm such as ahash for improved performance. The existing new(...) constructors are unchanged and delegate to the new ones using Default::default(), so there are no breaking changes.

Documentation and README updated with usage examples.

[jedisct1]

Thanks for working on this. I do not think this PR should merge yet.

The main issue is that the new custom-hasher support is only fully wired through the non-sharded cache. In src/sharded.rs:409, ShardedSieveCache::get_shard_index still hashes every key with DefaultHasher, even when the cache was created through new_with_hasher or with_shards_and_hasher. That means the supplied BuildHasher is not actually used for the first hash on every get, insert, remove, and contains_key call, which is exactly the hot path this feature is supposed to let callers tune. The same problem exists in src/weighted_sharded.rs:148, where WeightedShardedSieveCache also routes shards with DefaultHasher regardless of the user-provided hasher. As written, the sharded variants claim custom hasher support, but they still hard-code the default hashing algorithm for shard selection.

There is a second gap in the weighted implementation. WeightedSieveCache now takes a generic hasher parameter, but its secondary charged index remains a plain HashMap<K, usize> in src/weighted.rs:123, and it is still initialized with HashMap::new() in src/weighted.rs:167. So even in the non-sharded weighted cache, the custom hasher only applies to the inner SieveCache, not to all of the cache’s own hash-based bookkeeping. Since the PR description says this adds custom hash builder support to “all cache types,” that promise is not quite true today.

I did run the full test suite on the PR branch, and everything passed, which is a good sign for existing behavior. The problem is that there do not seem to be any tests exercising the new custom-hasher paths themselves, so these incomplete integrations slip through. I would want to see the custom hasher actually used consistently, or the scope of the feature and docs narrowed so they match what is really implemented.

This is an automated review by Swival (https://swival.dev) using only open-source models, with the goal of helping both the pull request author and the maintainers.

[ndrscodes]

Thanks for working on this. I do not think this PR should merge yet.

The main issue is that the new custom-hasher support is only fully wired through the non-sharded cache. In src/sharded.rs:409, ShardedSieveCache::get_shard_index still hashes every key with DefaultHasher, even when the cache was created through new_with_hasher or with_shards_and_hasher. That means the supplied BuildHasher is not actually used for the first hash on every get, insert, remove, and contains_key call, which is exactly the hot path this feature is supposed to let callers tune. The same problem exists in src/weighted_sharded.rs:148, where WeightedShardedSieveCache also routes shards with DefaultHasher regardless of the user-provided hasher. As written, the sharded variants claim custom hasher support, but they still hard-code the default hashing algorithm for shard selection.

There is a second gap in the weighted implementation. WeightedSieveCache now takes a generic hasher parameter, but its secondary charged index remains a plain HashMap<K, usize> in src/weighted.rs:123, and it is still initialized with HashMap::new() in src/weighted.rs:167. So even in the non-sharded weighted cache, the custom hasher only applies to the inner SieveCache, not to all of the cache’s own hash-based bookkeeping. Since the PR description says this adds custom hash builder support to “all cache types,” that promise is not quite true today.

I did run the full test suite on the PR branch, and everything passed, which is a good sign for existing behavior. The problem is that there do not seem to be any tests exercising the new custom-hasher paths themselves, so these incomplete integrations slip through. I would want to see the custom hasher actually used consistently, or the scope of the feature and docs narrowed so they match what is really implemented.

This is an automated review by Swival (https://swival.dev) using only open-source models, with the goal of helping both the pull request author and the maintainers.

Thanks for the quick review! You’re right, I’ll continue working on this. I think I’ll find the time tomorrow at the latest :)

[ndrscodes]

@jedisct1 I am already implementing the fixes as we speak. While i was looking through the codebase, I noticed that the charged HashMap in WeightedSieveCache is currently being built by calling HashMap::new() instead of HashMap::with_capacity(). Is this done on purpose, or should i include a fix for this?

[jedisct1]

I do not see a behavioral reason for charged to use HashMap::new() there.

Since charged mirrors the live cache entries one-for-one, using HashMap::with_capacity(capacity) would be consistent with how the inner cache preallocates its storage and should avoid a few unnecessary reallocations as the cache fills.

I would treat that as a small non-blocking improvement, not one of the issues holding this PR back. So if you are already touching WeightedSieveCache, I think it is fine to include the fix. If you want to keep the scope tight, it could also be a separate cleanup later.

If you do touch it as part of this PR, I would just make sure the change stays narrowly scoped and does not distract from the custom-hasher fixes.

[ndrscodes]

Hi @jedisct1. I implemented the changes. I also noticed the following issue: Due to the sharding in sharded cache versions, eviction will sometimes occur even though the capacity specified in the constructor is sufficient in theory. this occurs due to how the cache distributes the capacity amongst the shards. For example, a cache with a capacity of ten and a number of shards of 16 will have a capacity of 1 for the first ten shards and a capacity of zero for the remaining shards. I think this is out of scope for this PR, but it would definitely be worth mentioning in the docs, or every shard should have this capacity. Giving the capacity to every shard would incur a lot of memory overhead though. There also seems to be a flaky test (test_parallel_access) which will sometimes fail since 1 - 10 nodes are evicted before the test completes. If the docs are modified, I think the test could simply be modified to use a higher capacity.

[jedisct1]

I do not think this should merge yet.

The biggest remaining issue is that the weighted sharded variant still does not actually honor the
caller-provided hash builder for shard routing. In src/weighted_sharded.rs:148,
WeightedShardedSieveCache::get_shard_index still creates a DefaultHasher directly instead of using the
custom S passed through new_with_hasher and with_shards_and_hasher. That means get, insert, remove,
and contains_key on the weighted sharded cache all keep using the standard hash algorithm on the hot path even
when the API promises otherwise. The non-weighted sharded cache was fixed in src/sharded.rs:409, so this now
feels like an inconsistency rather than an intentional limitation.

There is also a test coverage gap around feature combinations. The new integration test file
tests/custom_hasher_test.rs:1 is not gated per test or per import, so it assumes sync, sharded, and
weighted are all enabled at once. On the PR branch, cargo test with default features passes, but cargo test --no-default-features fails to compile because that test still imports and references types behind disabled
feature flags. I also checked cargo test --no-default-features --features sync and cargo test --no-default-features --features sharded, and those fail for the same reason. Since this crate exposes feature
flags as part of its public surface, breaking non-default feature builds is a real regression.

The new tests also do not currently prove that the sharded caches are using the supplied hasher for shard
selection. In tests/custom_hasher_test.rs:11, CustomHasher::finish() always returns 0, so every key lands
in shard 0 whether get_shard_index uses the custom hasher or a hard-coded DefaultHasher. That is exactly why
the remaining bug in src/weighted_sharded.rs:148 slips through. I would suggest using a hasher whose
finish() depends on the bytes written and then asserting behavior that distinguishes the custom routing path
from the default one.

I ran the PR branch in a detached worktree. The default suite passed cleanly with cargo test, which is a good
sign for existing behavior, but the feature-matrix checks above exposed the compile regression. This is an
automated review by Swival (https://swival.dev) using only open-source models, with the goal of helping both the
pull request author and the maintainers.

[ndrscodes]

@jedisct1 the failing test is unrelated to my PR in my opinion. It's caused by the issue i described in my previous comment

[jedisct1]

Findings

• High: tests/custom_hasher_test.rs:8 unconditionally imports SyncSieveCache, so feature-specific builds without sync fail before the #[cfg(feature = "sync")] test gating can help. I reproduced this with cargo test --quiet --no-default-features, cargo test --quiet --no-default-features --features sharded, and cargo test --quiet --no-default-features --features weighted; all fail with no SyncSieveCache in the root. Moving that import behind #[cfg(feature = "sync")] or using sieve_cache::SyncSieveCache only inside the gated tests should fix the regression.

Notes

• The earlier custom-hasher wiring issues look addressed in the current head: ShardedSieveCache and WeightedShardedSieveCache now route shards through the configured BuildHasher, and WeightedSieveCache initializes charged with the provided hasher.
• The new custom hasher tests are much stronger now because they count hasher invocations and are mostly feature-gated correctly.
• I ran cargo test --quiet on the PR head and the default-feature suite passed: 51 + 6 + 6 + 42 + 86 tests.
• CI is currently reporting build as failed, which lines up with the feature-combination break above.

Merge recommendation: not quite ready yet. Once the unconditional SyncSieveCache import is gated and the failing feature builds are green, this looks close to mergeable from my review.

This is an automated review by Swival (https://swival.dev), using only open-source tooling.

[ndrscodes]

@jedisct1 i added a feature-gate for the import. Also, please look at my previous comment concerning the failing test. How would you like me to address this problem?

[jedisct1]

Non-default features still break:

  - cargo test --no-default-features → src/lib.rs - (line 107), (line 160), (line 232) fail
  - --features sync → lines 160 + 232 fail
  - --features weighted → lines 107 + 160 fail

[ndrscodes]

@jedisct1 I had to add feature guards to the README.md. Please note that on my system, these exact tests are also failing on the master branch.

[ndrscodes]

@jedisct1 could you please give this another review if you find the time?

[jedisct1]

The feature-gating issue looks fixed now: cargo test --no-default-features, --features sync, --features sharded, and --features weighted all pass locally, as does the default cargo test.

On the flaky test_parallel_access: I agree with your diagnosis. This is caused by per-shard capacity enforcement and uneven shard distribution, not by the custom-hasher feature itself. master now has ae27054 (Stabilize test_parallel_access against shard skew), which fixes the test by giving each shard enough headroom. The PR just needs to merge/rebase on current master to pick that up.

One remaining custom-hasher issue I still see is the SyncSieveCache -> ShardedSieveCache conversion in src/sharded.rs: it still rebuilds the sharded cache with Default::default() instead of preserving the source cache’s hasher instance. That means stateful custom hashers are discarded during conversion, and non-Default custom hashers cannot use this conversion at all because of the new S: Default bound. If preserving the exact hasher instance is out of scope or impractical with the current API, I’d at least prefer making that limitation explicit; otherwise, this conversion path should be adjusted so custom hasher support is consistent with the rest of the PR.

So from my side, the remaining action items are:

1. Rebase/merge current master to pick up the flaky test fix.
2. Address or document the hasher-preservation behavior in the sync-to-sharded conversion.

After that, this looks very close.