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

accounts-db: Use NoHashHasher in AccountStorageMap #4822

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

vadorovsky
Copy link
Member

@vadorovsky vadorovsky commented Feb 6, 2025

Problem

DashMap uses SipHash by default. AccountStorageMap uses Slot as a key and therefore it doesn't have any chance of collisions.

Summary of Changes

We can skip hashing the key entirely.

Ref #4276

@vadorovsky vadorovsky force-pushed the ahash-account-storage branch 3 times, most recently from 06c3639 to 013b3b1 Compare February 6, 2025 10:59
@vadorovsky vadorovsky marked this pull request as ready for review February 6, 2025 11:26
@brooksprumo brooksprumo requested a review from roryharr February 6, 2025 14:00
@brooksprumo
Copy link

Added @roryharr as a reviewer, since he's starting to work on similar things.

@vadorovsky vadorovsky force-pushed the ahash-account-storage branch from 013b3b1 to ba05292 Compare February 6, 2025 15:00
runtime/src/bank/serde_snapshot.rs Outdated Show resolved Hide resolved
@HaoranYi
Copy link

HaoranYi commented Feb 6, 2025

I have the same comments as brooks. Other than that, it looks good to me.

@vadorovsky vadorovsky force-pushed the ahash-account-storage branch from ba05292 to 4e6ceb0 Compare February 10, 2025 12:37
Copy link

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

Code looks good to me. Can you please update the PR title since we're not using AHash.

`DashMap` uses SipHash by default. `AccountStorageMap` uses `Slot` as a
key and therefore it doesn't have any chance of collisions. We can skip
hashing the key entirely.
@vadorovsky vadorovsky force-pushed the ahash-account-storage branch from 4e6ceb0 to 6d1a890 Compare February 10, 2025 16:17
@vadorovsky vadorovsky changed the title accounts-db: Use AHash in AccountStorageMap accounts-db: Use NoHashHasher in AccountStorageMap Feb 10, 2025
Copy link

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

:shipit:

Let's backport to v2.2 too.

Copy link

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

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

lgtm.

@vadorovsky vadorovsky merged commit cd04323 into anza-xyz:master Feb 10, 2025
58 checks passed
@vadorovsky vadorovsky deleted the ahash-account-storage branch February 10, 2025 17:46
@vadorovsky vadorovsky added the v2.2 Backport to v2.2 branch label Feb 11, 2025
Copy link

mergify bot commented Feb 11, 2025

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

mergify bot pushed a commit that referenced this pull request Feb 11, 2025
`DashMap` uses SipHash by default. `AccountStorageMap` uses `Slot` as a
key and therefore it doesn't have any chance of collisions. We can skip
hashing the key entirely.

(cherry picked from commit cd04323)
@t-nelson
Copy link

doesn't this change imply that hashmap is the wrong data structure to use here? Vec<Vec<Path>> would suffice

brooksprumo pushed a commit that referenced this pull request Feb 11, 2025
#4822) (#4920)

accounts-db: Use NoHashHasher in `AccountStorageMap` (#4822)

`DashMap` uses SipHash by default. `AccountStorageMap` uses `Slot` as a
key and therefore it doesn't have any chance of collisions. We can skip
hashing the key entirely.

(cherry picked from commit cd04323)

Co-authored-by: Michal Rostecki <vadorovsky@protonmail.com>
@brooksprumo
Copy link

doesn't this change imply that hashmap is the wrong data structure to use here? Vec<Vec<Path>> would suffice

This structure is mostly adding new entries, removing old entries, and doing lookups. We now only have one storage per slot, so we could probably remove the inner Vec. But scanning the vec to find a slot doesn't seem ideal. Maybe an IndexMap would be another option?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2.2 Backport to v2.2 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants