Use get_pinned for getting data from rocksdb#629
Conversation
Signed-off-by: Jacob Finkelman <YeomanYaacov@gmail.com>
Signed-off-by: Jacob Finkelman <YeomanYaacov@gmail.com>
Signed-off-by: Jacob Finkelman <YeomanYaacov@gmail.com>
WalkthroughG'day! This PR's a cracking optimization effort, mate. The changes swap owned Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (10)
🧰 Additional context used🧠 Learnings (7)📚 Learning: 2025-08-20T13:02:25.763ZApplied to files:
📚 Learning: 2025-05-21T18:58:48.631ZApplied to files:
📚 Learning: 2025-12-16T21:32:37.668ZApplied to files:
📚 Learning: 2025-09-06T09:16:25.025ZApplied to files:
📚 Learning: 2025-08-08T14:46:53.013ZApplied to files:
📚 Learning: 2025-08-08T14:35:35.562ZApplied to files:
📚 Learning: 2025-06-03T06:31:57.736ZApplied to files:
🧬 Code graph analysis (6)crates/amaru-stores/src/rocksdb/ledger/columns/pots.rs (6)
crates/amaru-stores/src/rocksdb/ledger/columns/accounts.rs (7)
crates/amaru-ledger/src/store/columns/mod.rs (7)
crates/amaru-stores/src/rocksdb/ledger/columns/slots.rs (1)
crates/amaru-stores/src/rocksdb/mod.rs (3)
crates/amaru-stores/src/rocksdb/ledger/columns/pools.rs (6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (19)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
KtorZ
left a comment
There was a problem hiding this comment.
Thanks for also fixing the iterator logic.
(This is a resubmit of #625, to see if CI will pass from a branch in this repo.)
A large number of short lived allocations come from copying data out of rocksdb. The
getanditerator_optmethods copy the data out of the database into a temporaryVec | Boxso that the Rust code can choose to keep the data as long as it wants. In our case, we immediately decode the data into more specific types, and then drop theVec | Box. So it would be more efficient for us to generate the specific types directly from the underlying buffer held by the database.The
getcan be replaced with aget_pinned. This requires changes in a number of places, but they're pretty mechanical.Unfortunately, there is no standard idiom for a iterator whose items can only be used until the next item is generated. The general problem is known as "lending iterator" and was one of the known limitations when Rust 1.0 was released. Rocksdb sorts this out by providing a raw interface to its iterators. We can write our own loop to read the raw data out of the database and convert it to our specific type. Once we've done that conversion we no longer have a lending problem and can construct a custom iterator type.
std::iter::from_fnmassively reduces the boilerplate.These kinds of short lived allocations sometimes anger allocators. This one was particularly suspicious because it occurred at the same time as the construction of the big
votescontainer disgust in the #594. Having now made the these changes and tested the result, no connection. These changes do not make a measurable difference to peak memory usage or throughput. :-( It does massively reduce the number of allocations and thereby the overhead of using memory profiling tools.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.