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

Replace domain string with enum for AccountStorageMap key #3677

Conversation

fxamacker
Copy link
Member

Updates #3584
Requires #3673

Description

This optimization further improves storage efficiency of domain payloads of PR #3664.

Thanks @turbolent 👍 for suggesting this optimization during PR review!

This PR replaces domain string, such as "storage", "public", "private", etc. with domain enum (integer) as the key for AccountStorageMap.

Also, this uses CBOR to store the domain integer in shortest form.


  • 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 replaces domain string, such as "storage",
"public", "private", etc. with domain enum (integer)
as the key for AccountStorageMap.

Also, this uses CBOR to store the domain integer in shortest form.

This optimization further improves storage efficiency
of domain payloads of PR 3664.
@fxamacker fxamacker changed the base branch from fxamacker/sync-feature-branch-combine-domain-payloads to feature/combine-domain-payloads-and-domain-storage-maps November 12, 2024 23:18
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!

interpreter/account_storagemap.go Outdated Show resolved Hide resolved
interpreter/account_storagemap.go Show resolved Hide resolved
This commit renames function convertKeyToDomain to
convertAccountStorageMapKeyToStorageDomain.
Copy link

Cadence Benchstat comparison

This branch with compared with the base branch onflow:feature/combine-domain-payloads-and-domain-storage-maps commit cc95223
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!

@fxamacker fxamacker merged commit 0abfa9a into feature/combine-domain-payloads-and-domain-storage-maps Nov 13, 2024
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants