Reduce storage register reads when using StorageFormatV2Enabled
#3683
Conversation
Currently, all domain registers are being read for a scenario that does not require it. When StorageFormatV2 feature flag is enabled, Storage.GetDomainStorageMap() reads all domain registers to determine if account is new account or in V1 format. These register reads are unnecessary for V1 accounts when: - the given domain register exists, or - the given domain register doesn't exist and createIfNotExists param is false. This commit modifies Storage.GetDomainStorageMap() to read the given domain register first (before iterating all domain registers) to reduce register reads. - If the given domain register exists, then account format is V1 and no more register reading is needed. - If the given domain register doesn't exist and createIfNotExists is false, then nil domain storage map is returned and no more register reading is needed. - Otherwise, account format (V1 account or new account) is determined by iterating all domain registers.
This commit adds tests for register reads from GetStorageMap() for account storage format v1 and v2.
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:feature/combine-domain-payloads-and-domain-storage-maps commit caaf254 Collapsed results for better readability
|
There was a problem hiding this comment.
Nice work improving this!
The changes look good, I left some very minor suggestions.
I'm having a bit of a harder time understanding when/where we have unnecessary register reads currently, and how the changes of the PR remove the unnecessary register reads, especially because it seems to add an additional read in one of the existing tests.
Could we maybe add a test case to feature/combine-domain-payloads-and-domain-storage-maps before this PR that demonstrates the inefficiency (unnecessary reads), and then this PR could would show more clearly how the unnecessary read(s) are removed?
| // Read contract domain register. | ||
| // Read returns no value. | ||
| { | ||
| owner: signerAddress[:], | ||
| key: []byte(common.StorageDomainContract.Identifier()), | ||
| }, |
There was a problem hiding this comment.
Is it expected that we're incurring an additional read here?
|
|
||
| const ( | ||
| storageFormatUnknown storageFormat = iota | ||
| storageFormatNew |
There was a problem hiding this comment.
What does "new" mean here? Isn't StorageFormatV2 already covering that?
| defer func() { | ||
| s.cacheIsV1Account(address, isV1) | ||
| }() | ||
| isCachedV1, isCachedV2 := s.getCachedAccountFormat(address) |
There was a problem hiding this comment.
Maybe getCachedAccountFormat can just return a (format storageFormat, known bool)?
| if s.isV2Account(address) { | ||
| return StorageFormatV2 | ||
| } | ||
|
|
||
| // Check if account is v1 (by reading given domain register). | ||
|
|
||
| if s.hasDomainRegister(address, domain) { | ||
| return storageFormatV1 | ||
| } |
There was a problem hiding this comment.
Good idea of splitting up the checks 👍
|
|
||
| // At this point, account is either new account or v1 account without given domain register. | ||
|
|
||
| if s.isV1Account(address) { |
There was a problem hiding this comment.
I was at first confused why onlyCheckSpecifiedDomainForV1 bailed out before even calling isV1Account, but I guess onlyCheckSpecifiedDomainForV1 applies to only calling hasDomainRegister, which is for V1, right?
| func (s *Storage) getCachedAccountFormat(address common.Address) (isV1 bool, isV2 bool) { | ||
| isV1, cached := s.cachedV1Accounts[address] | ||
| if !cached { | ||
| return false, false | ||
| } | ||
| return isV1, !isV1 | ||
| } |
There was a problem hiding this comment.
Maybe return the storage format, and if it is known/cached
| func (s *Storage) getCachedAccountFormat(address common.Address) (isV1 bool, isV2 bool) { | |
| isV1, cached := s.cachedV1Accounts[address] | |
| if !cached { | |
| return false, false | |
| } | |
| return isV1, !isV1 | |
| } | |
| func (s *Storage) getCachedAccountFormat(address common.Address) (format storageFormat, known bool) { | |
| isV1, cached := s.cachedV1Accounts[address] | |
| if !cached { | |
| return storageFormatUnknown, false | |
| } | |
| if isV1 { | |
| return storageFormatV1, true | |
| } else { | |
| return StorageFormatV2, true | |
| } | |
| } |
|
|
||
| // isV1Account returns true if given account is in account storage format v1 | ||
| // by checking if any of the domain registers exist. | ||
| func (s *Storage) isV1Account(address common.Address) (isV1 bool) { |
There was a problem hiding this comment.
Can call hasDomainRegister now in the loop
| return storageFormatV1 | ||
| } | ||
|
|
||
| return storageFormatNew |
There was a problem hiding this comment.
Could this just be storageFormatV2?
Updates #3584
Currently in PR #3678, all domain registers are being read for a scenario that does not require it. This PR reduces register reads as discussed.
Problem
When
StorageFormatV2feature flag is enabled,Storage.GetDomainStorageMap()reads all domain registers to determine if account is new account or in v1 format.These register reads are unnecessary when:
createIfNotExistsparam is false.Solution
This PR modifies
Storage.GetDomainStorageMap()to read the given domain register first (before iterating all domain registers) to reduce register reads.If the given domain register exists, then account format is v1 and no more register reading is needed.
If the given domain register doesn't exist and
createIfNotExistsis false, thennildomain storage map is returned and no more register reading is needed.Otherwise, account format (v1 account or new account) is determined by iterating all domain registers.
Caveat
This change increases code complexity in order to avoid reading all domain registers for this scenario. I added more comments to offset some of this tradeoff. For more context, see comment thread at #3678.
UPDATE:
I can add more tests for register reading after sync with Cadence team about this approach.I added more tests in case we don't get chance to sync with Cadence team this week.
masterbranchFiles changedin the Github PR explorer