Skip to content
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

[Account Storage Maps] Add more tests for register reads for GetDomainStorageMap() #3694

Conversation

fxamacker
Copy link
Member

Updates #3584

This PR adds more tests for register reads from GetDomainStorageMap() for account storage format v1 and v2.

Context

This PR addresses comment in PR #3683. Thanks @turbolent for great suggestion! 👍

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?

This PR updates these tests ported from #3683:

  • TestGetDomainStorageMapRegisterReadsForNewAccount
  • TestGetDomainStorageMapRegisterReadsForV1Account
  • TestGetDomainStorageMapRegisterReadsForV2Account

While at it, I also updated these tests to include unique register reads (in addition to sequence of register reads). Same register can be read multiple times and all reads are included in the sequence. Since subsequent register reads are from ledger cache, unique register reads are more important for performance reasons.

Unique register reads differs from that in PR #3683 ("Reduce storage register reads when using StorageFormatV2Enabled") when using StorageFormatV2Enabled:


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

This commit adds tests for register reads from GetStorageMap()
for account storage format v1 and v2.
@fxamacker fxamacker self-assigned this Nov 25, 2024
Copy link

Cadence Benchstat comparison

This branch with compared with the base branch onflow:feature/combine-domain-payloads-and-domain-storage-maps commit caaf254
The command for i in {1..N}; do go test ./... -run=XXX -bench=. -benchmem -shuffle=on; done was used.
Bench tests were run a total of 7 times on each branch.

Collapsed results for better readability

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thank you for adding more tests 🙏

runtime/storage_test.go Outdated Show resolved Hide resolved
runtime/storage_test.go Outdated Show resolved Hide resolved
@turbolent turbolent changed the title Add more tests for register reads for GetDomainStorageMap() [Account Storage Maps] Add more tests for register reads for GetDomainStorageMap() Nov 25, 2024
@fxamacker fxamacker enabled auto-merge November 25, 2024 23:06
@fxamacker fxamacker merged commit 157d322 into feature/combine-domain-payloads-and-domain-storage-maps Nov 25, 2024
7 of 9 checks passed
@Milad335t
Copy link

@fxamacker

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants