Merge atree register inlining v1.0 #3502
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
This commit updates Cadence to use new Atree API - Array.SlabID() - OrderedMap.SlabID() - SlabID - SlabIndex - etc. For more info, see Atree PRs: - onflow/atree#322 - onflow/atree#323 - onflow/atree#324
Currently, Array.SlabID() and OrderedMap.SlabID() are used as identifier to track resources, etc because slab IDs are guaranteed to unique. However, atree slab ID should be only used to retrieve slabs (registers) from storage. Also, when Atree register inlining is implemented in the future, some resources may not be stored in separate slabs, so they will not have slab IDs anymore. This commit uses Array.ValueID() and OrderedMap.ValueID() to uniquely identify resources.
This commit updates Cadence to use newer version of onflow/atree with register inlining and data deduplication features. - Updated StorableDecoder callbacks - Updated StringAtreeValue to implement atree.ComparableStorable - Updated SomeStorable to implement atree.ContainerStorable - Updated these structs to implement TypeInfo interface: * compositeTypeInfo * EmptyTypeInfo * VariableSizedStaticType * ConstantSizedStaticType * DictionaryStaticType - Updated decodeStorable() to decode atree inlined array, map, and compact map - Updated to use ReadOnly iterators when possible - Updated to use NonReadOnly iterators
Co-authored-by: Bastian Müller <[email protected]>
Co-authored-by: Bastian Müller <[email protected]>
Co-authored-by: Supun Setunga <[email protected]>
…ows using read-only iterator
…eturn shallow copy
Currently (in master), there are 4 cases of SomeStorable encoding: - SomeStorable and its content are encoded inline - SomeStorable is encoded inline while its content is encoded in separate slab when content size exceeds size limit - SomeStorable and its content are encoded in separate slab when content size fits withinin size limit but SomeStorable plus its content size exceeds size limit - SomeStorable and its content are encoded across multiple slabs when there are multiple nested SomeStorable and some of them exceeds size limit Given this, adding Atree Inlining feature (onflow/atree PR 342) makes encoding even more complicated for SomeStorable when it contains inlined array or map. This commit simplifies encoding by removing the last 2 cases, so SomeStorable can be encoded in two ways: - if innermost value is too large, innermost value is encoded in a separate slab while SomeStorable wrapper is encoded inline with reference to slab containing innermost value. - otherwise, SomeStorable with innermost value is encoded inline. The above applies to both immutable innermost value (such as StringValue), and mutable innermost value (such as ArrayValue). Additionally, this commit also adds new efficient encoding for SomeStorable when it has multiple nested levels (SomeStorable containing SomeStorable).
Co-authored-by: Bastian Müller <[email protected]>
Cadence smoke test is reporting false alarm about "slab was not reachable". To summarize, storage.CheckHealth() should be called after array.DeepRemove(), rather than in the middle of array.DeepRemove(). CheckHealth() is called in the middle of array.DeepRemove() when: - array.DeepRemove() calls childArray1 and childArray2 DeepRemove() - DeepRemove() calls maybeValidateAtreeValue() - maybeValidateAtreeValue() calls CheckHealth() The details are more complex than this oversimplified summary. For more context, see comments/discussion on GitHub at #2882 (comment)
…idation at root of deep remove
Cadence smoke test reported another false alarm about "slab was not reachable". To summarize, storage.CheckHealth() should be called after DictionaryValue.Transfer() with remove flag, rather than in the middle of DictionaryValue.Transfer() with remove flag. The details are more complex than this oversimplified summary. For more context, see GitHub comments at #2882 (comment)
This commit adds a new parameter atRoot to Transfer(). Atree storage is validated only if both remove and atRoot parameters are true in ArrayValue.Transfer(), DictionaryValue.Transfer(), and CompositeValue.Transfer(). Currently in ArrayValue.Transfer(), DictionaryValue.Transfer(), and CompositeValue.Transfer() if remove parameter is true: - atree storage is validated - container slab is removed However, when transferring nested ArrayValue, DictionaryValue, and CompositeValue with remove flag, atree storage validation can fail because child slab is removed while parent container is not reset completely yet. This validation problem is similar to the previously fixed atree storage validation problem during nested DeepRemove().
If remove parameter is true in Transfer(), iterated values are removed so non-readonly iterators need to be used.
…inlining-v1.0 Update feature/atree-register-inlining-v1.0
This version merged atree inlining and deduplication feature branch into main branch after extensive testing.
…date-atree-register-inlining-v1.0
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:master commit e79cdf8 Collapsed results for better readability
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Had a final look through the changes, LGTM |
SupunS
approved these changes
Jul 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Merge the atree register inlining feature branch to master.
Amazing work @fxamacker 👏👏👏
masterbranchFiles changedin the Github PR explorer