feat: optimize allocations in writable CoW accounts#4846
feat: optimize allocations in writable CoW accounts#4846bmuddha wants to merge 3 commits intoanza-xyz:masterfrom
Conversation
This feature removes the unnecessary calls to Vec::reserve (AccountSharedData.data field) Instead when realloc is detected during deserialization of SVM buffer back into account, extra allocation happens on demand. This should decrease the pressure on allocator when transactions writing to the account do not reallocate, by avoiding allocation where possible.
|
The Firedancer team maintains a line-for-line reimplementation of the |
|
I believe some benchmarks could be run to make the case for this change. Maybe @seanyoung has some suggestions. |
|
Needs a careful review. I'll go over it a few times. |
| if additional > MAX_PERMITTED_DATA_INCREASE { | ||
| self.account | ||
| .reserve(additional.saturating_sub(MAX_PERMITTED_DATA_INCREASE)); | ||
| } |
There was a problem hiding this comment.
I know it was previously already the case and this condition just makes it better but the double reallocation does feel like a big waste. Especially in such case where the data size is know to be pretty big already.
I wonder if the introduction of a "make mut with additional size" or something of the sort would be acceptable.
Also i'm unsure if it is actually possible for additional to be > MAX_PERMITTED_DATA_INCREASE but since its been renamed from MAX_PERMITTED_DATA_LENGTH it may now/soon be the case.
There was a problem hiding this comment.
But overall feels like a very positive change - everything that can speed up execution is more than welcomed...
|
@seanyoung just a quick reminder on this PR, any progress in reviewing it? |
|
Hey guys, do you think this can be merged any time soon? Let me know if there're some issues that need to be addressed first |
|
This pull request is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
I think this has been implicitly addressed in #5871. See agave/transaction-context/src/transaction.rs Line 509 in 75a41e3 In short: If the program wants to grow the account we grow it by the max +10 KiB in one go as doing it in multiple increments would be slower for large accounts, which have to copy all the data to a new allocation. |
|
This pull request is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
5 participants
Problem
Unnecessary allocations in CoW accounts'
datafield, during the first write access from within the SVM. Potentially, instruction might just write to an account without reallocating, and thus premature call toVec::reserveintroduces an overhead which could be avoided.Summary of Changes
This performance optimization removes the unnecessary calls to Vec::reserve (on AccountSharedData.data field). Instead when realloc is detected during deserialization of SVM buffer back into account, extra allocation happens on demand. This should decrease the pressure on allocator when instructions writing to the account do not reallocate, by avoiding allocation where possible.