Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4163 +/- ##
==========================================
- Coverage 66.59% 65.85% -0.74%
==========================================
Files 2196 2111 -85
Lines 169208 160704 -8504
==========================================
- Hits 112676 105828 -6848
+ Misses 56391 54735 -1656
Partials 141 141
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview Shard lookups now accept Physical keys are built with new Benchmarks in Reviewed by Cursor Bugbot for commit e067bec. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A well-scoped allocation reduction on the flatkv state-view read path: the new AccountRow/StorageRow value types reproduce the existing row semantics exactly (compact/full account rows, tombstones, EmptyCodeHash substitution), the stack key buffer is sized exactly for the longest physical key, and the DeserializeCodeData aliasing is safe because Pebble reads and view iterators both clone and the write path never mutates a stored row in place. Findings are all non-blocking: duplicated row-parsing logic now living in two places, a couple of readability/reuse cleanups, and gaps in direct test coverage of the new exported functions.
Findings: 0 blocking | 7 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The new exported surface has no direct unit tests:
ParseAccountRow/ParseStorageRowerror branches (empty, bad version byte, wrong length — including the 49-vs-81 account case) andAppendEVMPhysicalKeyare only exercised indirectly through the state-view accessors. An equivalence test assertingAppendEVMPhysicalKey(nil, kind, k)equalsEVMPhysicalKey(kind, k)for every kind would also be the only coverage of its codehash/balance canonicalization branch, which no caller in this PR reaches (GetStoragepassesEVMKeyStorage,accountRowpassesEVMKeyAccount). - [suggestion]
benchViewhand-builds the balanceproto.KVPairwithkeys.BuildEVMKey(keys.EVMKeyBalance, ...)even thoughbalancePairalready exists intestutil_test.goalongside thenoncePair/storagePair/codePairhelpers it does use; reusing it keeps the balance key encoding in one place. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
|
|
||
| // ParseAccountRow validates data as an account row and returns a view over it. | ||
| func ParseAccountRow(data []byte) (AccountRow, error) { |
There was a problem hiding this comment.
[suggestion] ParseAccountRow/ParseStorageRow re-implement the validation that DeserializeAccountData/DeserializeStorageData already perform (non-empty, version byte, accepted lengths), and AccountRow.IsDelete/StorageRow.IsDelete restate the tombstone rule a second time. The row format now has two readers per type, and state_view.go keeps accountData/storageData alive solely for the Get path, so both definitions are live. Consider making the old types delegate (AccountData wrapping an AccountRow, or IsDelete forwarding), or moving Get onto the row types, so the tombstone rule and the length/version contract each have one definition.
|
|
||
| // AppendEVMPhysicalKey appends the physical key EVMPhysicalKey would build for kind and strippedKey | ||
| // to dst and returns the extended slice. | ||
| func AppendEVMPhysicalKey(dst []byte, kind keys.EVMKeyKind, strippedKey []byte) []byte { |
There was a problem hiding this comment.
[suggestion] AppendEVMPhysicalKey duplicates EVMPhysicalKey's codehash/balance canonicalization, prefix-byte lookup, and layout. EVMPhysicalKey can become a one-liner over it — return AppendEVMPhysicalKey(make([]byte, 0, len(keys.EVMStoreKey)+2+len(strippedKey)), kind, strippedKey) — which keeps its single allocation and its nil-on-unknown-kind return, and leaves one place where the physical key layout is defined. Right now a change to the layout has to be made twice, and the two can diverge silently since nothing asserts they agree.
| storage := v.storageData(ktype.StorageKey(ktype.Address(addr), ktype.Slot(key))) | ||
| if storage == nil { | ||
| var buf [physKeyBufLen]byte | ||
| physKey := ktype.AppendEVMPhysicalKey(buf[:0], keys.EVMKeyStorage, addr[:]) |
There was a problem hiding this comment.
[suggestion] GetStorage inlines key building, row parsing, the panic message, and the tombstone check, while the account read next to it is the named accountRow step. A matching storageRow(addr, key) (vtype.StorageRow, bool) would restore the symmetry and keep GetStorage a single named step, which is the shape the rest of this file uses.
|
|
||
| bytecode := make([]byte, len(data)-codeBytecodeStart) | ||
| copy(bytecode, data[codeBytecodeStart:]) | ||
| bytecode := data[codeBytecodeStart:] |
There was a problem hiding this comment.
[suggestion] The returned bytecode now aliases data, so a CodeData is only valid as long as its source bytes are — but neither DeserializeCodeData's nor GetBytecode's doc says so, and DeserializeMiscData still copies, so the package contract is now per-type and only discoverable by reading the bodies. The type comment covers mutation ("not safe to modify without first copying") but not lifetime. Worth stating the aliasing on DeserializeCodeData, since the non-flatkv callers (composite.convertFlatKVNodes, seidb evm_logical_digest) hand the slice on to code that has no view of where it came from.
| // 8 KiB of code, so each benchmark measures a cache-hit read. | ||
| func benchView(b *testing.B) (gigatypes.StateView, gigatypes.Address) { | ||
| b.Helper() | ||
| cfg := config.DefaultTestConfig(&testing.T{}) |
There was a problem hiding this comment.
[suggestion] config.DefaultTestConfig(&testing.T{}) calls TempDir() on a zero-value testing.T: it happens to work today, but the directory it creates is registered with that fake T's cleanup list, which never runs, so each benchmark invocation leaks an empty directory under the system temp dir (the real data dir is then overwritten on the next line). Widening DefaultTestConfig to testing.TB and passing b removes both the leak and the dependency on zero-value testing.T internals.
1 participant
Every typed read on the flatkv
StateView(GetStorage,GetBalance,GetNonce,GetCodeHash,AccountExists) made four heap allocations even on a cache hit: two to build the physical key (StorageKeythenEVMPhysicalKey), one for thestring(key)conversion passed intoshard.lookupVersionedRLocked(the compiler only elides that conversion when it is used directly as a map index), and one for the*AccountData/*StorageDatawrapper returned by the deserializer; compact account rows paid a fifth for the 81-byte expansion copy.GetCodeadditionally copied the whole bytecode out of the row, so an 8 KiB contract cost ~8.3 KB and ~2.4 µs per read.The shard lookup now takes the key as
[]byteand indexes the map withstring(key)inline. The view builds the physical key withktype.AppendEVMPhysicalKeyinto a fixed-size stack buffer, and reads rows through the newvtype.AccountRow/vtype.StorageRowvalue types, which alias the row bytes (accepting both compact and full account rows) and return fields by value instead of allocating a mutable wrapper.DeserializeCodeDatano longer copies the bytecode; the returned slice aliases the row, which is consistent with the existing contract that values returned by the store and its iterators may point at store memory. On the cache-hit benchmarks added here,GetStoragegoes from 210 ns / 216 B / 4 allocs to 88 ns / 64 B / 1 alloc,GetBalancefrom 169 ns / 176 B / 4 allocs to 80 ns / 64 B / 1 alloc, andGetCode(8 KiB) from 2393 ns / 8299 B / 4 allocs to 102 ns / 104 B / 3 allocs. The remaining allocation on the fixed-width paths is the key buffer escaping through theview.Viewinterface call.scripts/ramtest.sh ./sei-db/state_db/sc/flatkv/... ./sei-db/db_engine/view/... ./sei-db/state_db/ss/composite/...passes unchanged;golangci-lint runandgolangci-lint fmt --diffare clean.benchstat,
-count 10, origin/main vs this branch: