-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Buffer account's fields for hash #33788
Conversation
Codecov Report
@@ Coverage Diff @@
## master #33788 +/- ##
=========================================
- Coverage 81.8% 81.8% -0.1%
=========================================
Files 806 806
Lines 217859 217865 +6
=========================================
- Hits 178239 178229 -10
- Misses 39620 39636 +16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat idea. Since the buffer is only 80 bytes, we could probably allocate the buffer on the stack and that should shave more time off too (avoid the heap allocation).
Also, are the computed hash values equivalent? |
yeah. good idea. pushed a commit to use smallvec for the buffer. yeah. the hash matches. |
accounts-db/src/accounts_db.rs
Outdated
@@ -6219,31 +6220,38 @@ impl AccountsDb { | |||
} | |||
let mut hasher = blake3::Hasher::new(); | |||
|
|||
hasher.update(&lamports.to_le_bytes()); | |||
// allocate 128 bytes buffer on the stack | |||
let mut buffer = SmallVec::<[u8; 128]>::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 128 bytes? Looks like the largest we need is 65 bytes? Does SmallVec require the value to be a power of 2?
Looking at the source, I think SmallVec will accept 96
here too. Alternatively, we can use the const_generics
feature and that looks like it'll support arrays of any size:
https://docs.rs/smallvec/latest/src/smallvec/lib.rs.html#772-778
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup. 96 can be used.
#[cfg(any(not(feature = "const_generics"), doc))]
[impl_array!](https://docs.rs/smallvec/latest/src/smallvec/lib.rs.html#2406-2416)(
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
26, 27, 28, 29, 30, 31, 32, 36, 0x40, 0x60, 0x80, 0x100, 0x200, 0x400, 0x600, 0x800, 0x1000,
0x2000, 0x4000, 0x6000, 0x8000, 0x10000, 0x20000, 0x40000, 0x60000, 0x80000, 0x10_0000
);
With 128-byte buffer, we can optimize for small accounts, where data + fields fits into the buffer - hash them all at once. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done. Lgtm
|
Problem
Hashing individual fields of the account can be optimized by buffering those
fields and hash them together. Given larger buffer, the hasher can do better
with SIMD parallelism.
https://github.com/HaoranYi/my_bench/blob/master/benches/mybench.rs
Summary of Changes
buffer individual accounts fields together for hashing
Fixes #