-
Notifications
You must be signed in to change notification settings - Fork 137
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
Integrate with atree inlining and data deduplication #2882
Integrate with atree inlining and data deduplication #2882
Conversation
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
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:feature/atree-register-inlining commit fd29ad8 Collapsed results for better readability
|
c8cc585
to
ca1a9ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
@@ -18043,7 +18059,7 @@ func (v *DictionaryValue) ForEachKey( | |||
} | |||
|
|||
iterate := func() { | |||
err := v.dictionary.IterateKeys( | |||
err := v.dictionary.IterateReadOnlyKeys( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure that the procedure
passed to DictionaryValue.ForEachKey
does not mutate? I'd be conservative and switch to a read-only iterator after we ensured it. It might also be good to indicate that in the name then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure that the procedure passed to DictionaryValue.ForEachKey does not mutate?
Hmm, do you mean the procedure might mutate map value by iterated key?
Using a non-readonly vs readonly iterator probably wouldn't make a difference here because mutation through iterator is done by callback in returned map value. Since procedure only processes map key, mutating map value is not possible by callback. Actually, atree only has readonly iterators for map keys for the same reason.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[...] because mutation through iterator is done by callback in returned map value
Not quite sure what that means. Do you maybe have an example?
mutating map value is not possible by callback
The callback is free to do anything, including removing keys, inserting new keys, mutating values in the dictionary, etc.
I guess your point is still that mutable vs read-only iterator doesn't matter?
@@ -28,6 +28,7 @@ type StringAtreeValue string | |||
|
|||
var _ atree.Value = StringAtreeValue("") | |||
var _ atree.Storable = StringAtreeValue("") | |||
var _ atree.ComparableStorable = StringAtreeValue("") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this also needed for Uint64AtreeValue
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm mistaken, it probably isn't needed because:
atree.ComparableStorable
is only needed foratree.OrderedMap
keys in CadenceCompositeValue
StringAtreeValue
seems to be the only key type used byCompositeValue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringAtreeValue
is also used for "non-interpreter.Value
" storage, i.e. for the atree ordered maps used for account storage, and Uint64AtreeValue
is only used for those.
For example, saving a value to /storage/foo
actually is implemented as storing a value inside of the account storage's ordered map for the storage
domain, under key StringAtreeValue("foo")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was discussed in today's meeting and it isn't for Uint64AtreeValue
needed as mentioned above.
Co-authored-by: Bastian Müller <bastian@turbolent.com>
af06ef7
to
bc7b622
Compare
Co-authored-by: Bastian Müller <bastian@turbolent.com>
Co-authored-by: Supun Setunga <supun.setunga@gmail.com>
…alues
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## feature/atree-register-inlining #2882 +/- ##
===================================================================
+ Coverage 79.51% 79.52% +0.01%
===================================================================
Files 336 336
Lines 79191 79560 +369
===================================================================
+ Hits 62969 63273 +304
- Misses 13916 13974 +58
- Partials 2306 2313 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…ows using read-only iterator
…tion in tests
…eturn shallow copy
@turbolent Thanks for reviewing commit 1ecf35c on Friday! Today is Canadian holiday and Friday was US holiday, maybe we can sync about this before more holidays come up. 😄
Unless I missed something, I think we can skip incorporating all of the separate suggestions related to the last feedback. However, my reasons vary for some suggestions and I would like to get your thoughts about some new ideas/questions. 🙏 I will summarize here instead of replying to each suggestion individually because they are related. There are two main scenarios in your comments:
First Scenario
cadence/runtime/interpreter/value.go Lines 2769 to 2783 in 1ecf35c
Second ScenarioThis is more involved because:
cadence/runtime/stdlib/account.go Lines 1072 to 1111 in 1ecf35c
Alternative Suggestion for Second ScenarioWould it make sense to do these instead?
So Thoughts? |
TODO for this PR:
TODO for another PR, ideally before:
|
If remove parameter is true in Transfer(), iterated values are removed so non-readonly iterators need to be used.
- Renamed atRoot param to hasNoParentContainer for Transfer() and DeepRemove() - Added comments for each instances of hasNoParentContainer for Transfer() - For SomeValue.Transfer() and PublishedValue.Transfer(), use received hasNoParentContainer argument as inner value's Transfer() parameter
For SomeValue.DeepRemove(), PathCapabilityValue.Transfer(), and IDCapabilityValue.DeepRemove(), use received hasNoParentContainer argument as inner value's DeepRemove() parameter. Also, add comments for hasNoParentContainer in DeepRemove().
Hey @turbolent, thanks for a great meeting this morning to go over this PR again and also cover other issues in
Thanks for posting these TODOs from our meeting! 👍
Done in commit: Rename atRoot param for Transfer() and DeepRemove()
Done in commit: Use Atree non-readonly iterators in Transfer()
Done in commit: Use hasNoParentContainer for inner value DeepRemove() I hope you don't mind, 🙏 I was already there and wanted to have this PR wrapped up.
Thanks for tackling those outside this PR! 👍 |
if nestedLevels == 1 { | ||
return s.encode(e) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this also be handled as part of encodeMultipleNestedLevels (instead of returning error there) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for delay in reply, I missed this comment. 🙏
Maybe not in this case because encoding format is differently for nestedLevel == 1
(using encode()
) vs nestedLevel > 1
(using encodeMultipleNestedLevels()
):
- if nested level == 1, only innermost unwrapped storable is encoded
- if nested level > 1, then 2-element array is encoded: first element is nested levels, second element is innermost unwrapped storable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
The latest commits look good 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double checked the switches of iterators from mutating to read-only ones, LGTM in that regard 👍
valueComparator := newValueComparator(interpreter, locationRange) | ||
hashInputProvider := newHashInputProvider(interpreter, locationRange) | ||
|
||
iterator, err := v.dictionary.Iterator(valueComparator, hashInputProvider) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to use a mutable iterator here, because the values that are returned by the iterator might get mutated, so need to have the parent notification callback set up, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. 👍
@fxamacker Do you happen to remember what I meant with
I vaguely remember we discussed how there's a potential problem with that approach in the inlining version |
@turbolent Yeah, I remember that too. For context:
Part of the discussion to simplify
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work @fxamacker! 👏 👏 👏
…te-with-atree
deffd50
into
feature/atree-register-inlining
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)
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)
Closes #2809
Updates onflow/atree#292
This commit updates Cadence to use newer version of onflow/atree with register inlining and data deduplication features introduced by onflow/atree#342 and onflow/atree#345.
This PR requires onflow/atree#352.
Changes
Updated StorableDecoder callbacks
Updated StringAtreeValue to implement atree.ComparableStorable
Updated SomeStorable to implement atree.ContainerStorable
Updated these structs to implement TypeInfo interface:
Updated decodeStorable() to decode atree inlined array, map, and compact map
Updated to use ReadOnly iterators when possible
Updated to use NonReadOnly iterators
TODO
Cadence team 🙏 will need to make additional changes:
*Interpreter
andLocationRange
to some functions in order to use non-readonlyatree.OrderedMap
iteratorsatree.Array
andatree.OrderedMap
and needs to implementatree.ContainerStorable
interfacemaster
branchFiles changed
in the Github PR explorer