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

Optimize max map value size to reduce number of registers #314

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

fxamacker
Copy link
Member

Closes #313
Updates #296 #292 onflow/flow-go#1744

Description

Previously, both max map key size and max map value size were the same (about half of max map element size). However, key size can be much smaller than max limit and max value size didn't benefit from smaller key.

Optimize this by computing max map value size to subtract encoded key size from max map element size. So large value can be stored along with small key to reduce number of registers.

While at it, also replace exported settings with exported functions.


  • Targeted PR against main 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

Previously, both max map key size and max map value size were the same
(about half of max map element size).  However, key size can be much
smaller than max limit and max value size doesn't benefit from smaller key.

Improve this by computing max map value size to subtract encoded key size
from max map element size.  So large value can be stored along with
small key to reduce number of slabs.

While at it, also replace exported settings with exported functions.
@fxamacker fxamacker added performance improvement breaking change changes to API that can break programs using Atree's API labels Jun 26, 2023
@fxamacker fxamacker requested a review from ramtinms as a code owner June 26, 2023 23:28
@fxamacker fxamacker self-assigned this Jun 26, 2023
@fxamacker fxamacker requested a review from turbolent as a code owner June 26, 2023 23:28
@codecov-commenter
Copy link

Codecov Report

Merging #314 (3c855fb) into main (bc0184e) will decrease coverage by 0.02%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##             main     #314      +/-   ##
==========================================
- Coverage   64.61%   64.59%   -0.02%     
==========================================
  Files          14       14              
  Lines        7991     7997       +6     
==========================================
+ Hits         5163     5166       +3     
- Misses       2152     2156       +4     
+ Partials      676      675       -1     
Impacted Files Coverage Δ
array_debug.go 51.65% <0.00%> (ø)
settings.go 81.25% <60.00%> (-11.06%) ⬇️
array.go 69.86% <100.00%> (ø)
basicarray.go 49.78% <100.00%> (ø)
map.go 66.79% <100.00%> (+0.09%) ⬆️

... and 1 file with indirect coverage changes

@turbolent
Copy link
Member

Nice! Does this need a state migration?

@fxamacker
Copy link
Member Author

fxamacker commented Jun 27, 2023

Nice! Does this need a state migration?

@turbolent Yes, this is part of prep work for Atree Register Inlining (#292) which requires migration anyway.

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.

Great idea and improvement!

A lot of code will benefit from this, as it is very common that the keys are much smaller than the values 👌

@fxamacker fxamacker merged commit a996413 into main Jun 29, 2023
fxamacker added a commit to onflow/cadence that referenced this pull request Jun 29, 2023
This commit updates Cadence to use new Atree API
- Array.Get()
- OrderedMap.Get()
- MaxInlineArrayElementSize()
- MaxInlineMapKeySize()

For more info, see Atree PRs:
- onflow/atree#314
- onflow/atree#316
- onflow/atree#318
turbolent pushed a commit to onflow/cadence that referenced this pull request Jan 26, 2024
This commit updates Cadence to use new Atree API
- Array.Get()
- OrderedMap.Get()
- MaxInlineArrayElementSize()
- MaxInlineMapKeySize()

For more info, see Atree PRs:
- onflow/atree#314
- onflow/atree#316
- onflow/atree#318
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change changes to API that can break programs using Atree's API improvement performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Max map value size can be optimized to reduce number of registers
3 participants