Atree register inlining for v1.0 #3048
Conversation
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 <bastian@turbolent.com>
Co-authored-by: Bastian Müller <bastian@turbolent.com>
Co-authored-by: Supun Setunga <supun.setunga@gmail.com>
…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 <bastian@turbolent.com>
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.
Entitlement migration was broken because new dictionary value was created with zero address using NewDictionaryValue(). This commit fixes entitlement migration by creating new dictionary value with owner address using NewDictionaryValueWithAddress().
|
@fxamacker Great work fixing the migrations! 👏 Out of curiosity: why were the migrations working in v0.42 without those equality functions? 🤔 |
@turbolent cadence/runtime/interpreter/value.go Lines 16665 to 16675 in c8b0110 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/atree-register-inlining-v1.0 #3048 +/- ##
========================================================================
+ Coverage 80.60% 80.62% +0.02%
========================================================================
Files 379 379
Lines 91151 91604 +453
========================================================================
+ Hits 73468 73854 +386
- Misses 15049 15110 +61
- Partials 2634 2640 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Those equality functions are for Cadence 1.0 migration and are not part of Cadence v0.42 migration with atree inlining. |
|
The only remaining TODO here is to fix one of the subtests ("resource, move from array to array") in It's been a while, but as far as I can remember I added this test case in the atree register inlining feature to "prepare" for the changes to come, to basically have a regression test for "complex" operations on containers, like moving children between different containers. I think I need to update it to the new reference semantics in 1.0, i.e. references becoming invalid when the resource is moved. @SupunS does this sound about right? |
|
Adjusted and unskipped the test case in 6713e43 |
|
adjustment for the test 6713e43 looks correct 👍 |
There was a problem hiding this comment.
Nice! 👍 I reviewed mostly non-test code against the other branch. I manually looked for any commits that might have been missed because the git range-diff had issues (maybe my bad).
There are some places in value.go that can use readonly iterators, such as ArrayValue.IsImportable(). I can open a separate PR for those, which would also address issue #2836.
This commit bumps atree version to benefit from deduplication of atree inlined Cadence dictionary type info. For more info, see onflow/atree#369
…inlining-v1.0 Bump atree version in Cadence v1.0 for new features (e.g. deduplication of Cadence dictionary type info)
Currently, we are not sure if all uses can be guaranteed to be readonly in two functions, so this commit uses atree non-readonly iterators in: - CompositeValue.ForEachFieldName - DictionaryValue.IterateKeys Also added TODO to determine if all uses can be guaranteed to be readonly for these, which would allow us to revert this change.
…rators Use atree readonly iterators when mutation of values is not needed
turbolent
merged commit b591a0d
into
feature/atree-register-inlining-v1.0
Description
Rebase of
feature/atree-register-inlining-v0.42on top ofmaster.I attempted merging
masterand resolving conflicts multiple times, but the sheer amount of conflicts and differences made it impossible for me. Rebasing allowed making semantically equivalent changes, step-by-step.masterbranchFiles changedin the Github PR explorer