Atree Register Inlining and Data Deduplication #342

fxamacker · 2023-09-14T17:35:04Z

Updates epics #292 onflow/flow-go#1744
Updates #296 #297 #347 #348 onflow/cadence#1854
Required by Cadence Integration with Atree onflow/cadence#2809
Required by Atree Storage Migration:

Storage migration for Atree register inlining flow-go#4576 lists migration TODOs.
Atree storage migration flow-go#4633. Dry-runs can begin migration tests and optimizations without Atree Inlining but will eventually need to include this PR before spork.

NOTE: some of the above updated issues will be closed by this PR:

after integration with Cadence (in onflow/cadence repo) and
after long-running smoke tests run longer (some have been running locally since Oct 5 and Oct 6).

Problem

Memory use on execution nodes, register count, and growth rate can be improved.

Currently, every array or map are stored in its own slab and parent slab refers to child array or map by SlabID.

The current approach can lead to many small slabs, especially for Cadence data structures with multiple nested levels.

Another aspect is duplicate data, especially when encoding nested Cadence composites. The design of atree inlining already requries changing the encoding format, so use this opportunity to also deduplicate data when encoding nested composites.

Changes

Reduce register count by inlining and optimize encoding size by deduplicating Cadence composite types within slabs.

Inlining. This commit inlines child array/map in parent slab when:

child array/map fits in one slab (root slab is data slab)
encoded size of inlined child array/map is less than the max inline size limit enforced by parent

Data deduplication. This commit optimizes encoding size by:

reusing inlined array types
reusing seed, digests, and field names of inlined composite types

Also update debugging code to handle inlined array/map element.

TODO

In this PR

add more tests
cleanup some of the tests

Atree Inlining Part 2

Open new PR to add support for inlinability of child values returned by iterator (requires PR #345)

Cadence Integration

Open new PR in onflow/cadence to integrate with Cadence.

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

Currently, every array or map are stored in its own slab and parent slab refers to child array or map by SlabID. The current approach can lead to many small slabs, especially for Cadence data structures with multiple nested levels. This commit inlines child array/map in parent slab when: - child array/map fits in one slab (root slab is data slab) - encoded size of inlined child array/map is less than the max inline size limit enforced by parent This commit optimizes encoding size by: - reusing inlined array types - reusing seed, digests, and field names of inlined composite types Also update debugging code to handle inlined array/map element.

Currently, in parentUpdater callback, parent array/map resets same child value. Child value ID should match overwritten SlabIDStorable or Slab.SlabID(). This commit adds check to make sure same child value is being reset.

codecov-commenter · 2023-09-17T20:52:20Z

Codecov Report

Attention: 1110 lines in your changes are missing coverage. Please review.

Comparison is base (1e6ec55) 64.93% compared to head (5f1de0a) 62.62%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #342      +/-   ##
==========================================
- Coverage   64.93%   62.62%   -2.31%     
==========================================
  Files          14       15       +1     
  Lines        8811    10559    +1748     
==========================================
+ Hits         5721     6613     +892     
- Misses       2356     3004     +648     
- Partials      734      942     +208

Files	Coverage Δ
encode.go	`80.72% <100.00%> (+4.25%)`	⬆️
storable.go	`59.37% <ø> (ø)`
basicarray.go	`49.38% <70.00%> (+0.01%)`	⬆️
storage.go	`73.09% <52.00%> (-0.60%)`	⬇️
typeinfo.go	`60.32% <60.32%> (ø)`
map_debug.go	`43.65% <49.34%> (-8.51%)`	⬇️
array_debug.go	`47.44% <49.04%> (-4.67%)`	⬇️
array.go	`68.10% <62.09%> (-1.97%)`	⬇️
map.go	`65.30% <58.52%> (-1.78%)`	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

turbolent · 2023-09-18T19:30:27Z

Fantastic work Faye! 👏

Some notes from the first review session:

It would be good to document that the "parent pointers" are not stored physically, but are actually functions, in memory (and when they are set). Maybe add some high-level comment to the code and indicate it in the diagrams; they usually show "stored" physical pointers ("storage IDs" (now "slab ID" / "value ID", right?))
Maybe document (if not already) that the inlining approach is to not inline all slabs that are "inlinable" in the sense of fitting inside of the parent, but that inlinability is determined "as a whole" for a whole value. The example was that, a small array is inlinable if it is just a single data slab, but a larger array consisting of a meta slab and some data slabs is not inlinable, even when the meta slab could fit into the parent
Maybe document (if not already), that "slab ID" == "value ID", i.e. they share a namespace, and an ID might be used for a slab, but it might also not. Currently, a slab ID could be checked to exist
As discussed, we might also want to update the child' parent callback when the child is inserted into the container (array and map), e.g. in the Set and Insert operations.

I'll do a proper code review after the next review session. So far the work looks great, well done! 👏

ramtinms

🚢 the logic and key parts of the changes look good to me, I did not check all the lines as we have a very good coverage by the tests.
Besides the suggestions I put in the code, I suggest also adding this logic to the stress test or tests to check for register allocation leaks (register falls behind), especially when we continuously move from large mutable to inlineable ones.

p.s. (cc: @janezpodhostnik ) I think we need higher-level testing for applications like the FVM environment but that's out of the scope of this repo. we might re-use the NFT tracker reporter to check the health before and after migration. and simulate some transactions to check the changes (especially re-sizing logic).

ramtinms · 2023-09-18T17:20:12Z

storable.go

+type EquatableStorable interface {
+	Storable
+	// Equal returns true if the given storable is equal to this storable.
+	Equal(Storable) bool


ramtinms · 2023-09-18T17:23:02Z

storage.go

+	copy(id[:], sid.address[:])
+	copy(id[8:], sid.index[:])


maybe we just add a warning that if we change the size of the address or index, we could have a problem here.

Better yet, consider using unsafe.Sizeof for the definition of ValueID above, in the range expression here. See for example: https://goplay.tools/snippet/EtdTvPNnIvF

ramtinms · 2023-09-18T17:24:06Z

typeinfo.go

 	"github.com/fxamacker/cbor/v2"
 )

 type TypeInfo interface {
 	Encode(*cbor.StreamEncoder) error
+	IsComposite() bool
+	ID() string
+	// TODO: maybe add a copy function because decoded TypeInfo can be shared by multiple slabs if not copied.


is this still a relevant TODO?

is this still a relevant TODO?

Yes, good catch. The TODO was a precaution that was postponed to get PR reviews started on the other parts.

It's done in 72d3614. TypeInfo is copied in the commit as a precaution because TypeInfo can be shared among inlined slabs referring to the same type and any changes to one may affect all others if not copied.

ramtinms · 2023-09-18T18:59:05Z

map.go

+		firstKey: elements.firstKey(),
+	}
+
+	// TODO: does extra data needs to be copied?


is this still a valid TODO?

is this still a valid TODO?

Yes 👍 See comment above and this is also done in the same commit 72d3614.

array.go

typeinfo.go

This commit adds callback notification in the set or inserted child element in array. So if child element is modified after Array.Set() or Array.Insert(), changes to child element is properly reflected in the parent array. This commit doesn't appear to be needed by current version of Cadence but it helps reduce Atree's dependence on an implementation detail of Cadence.

This commit adds callback notification in the set child element in map. So if child element is modified after OrderedMap.Set() , changes to child element is properly reflected in the parent map. This commit doesn't appear to be needed by current version of Cadence but it helps reduce Atree's dependence on an implementation detail of Cadence.

Currently, deduplication feature in PR 342 (not merged yet) does not support Cadence attachments. Add support for Cadence attachments and other future use cases by using type ID and sorted field names instead of field count. While at it, also refactor encoding composite type to reduce traversal and type assertion.

Decoded ExtraData (including TypeInfo) can be shared by all inlined slabs. This commit creates a new ExtraData with copied TypeInfo for inlined slabs to prevent accidental mutation.

Decoded ExtraData (including keys) can be shared by all inlined composite referring to the same type. This commit copies key for inlined composite to prevent accidental mutation.

turbolent · 2023-10-03T20:09:36Z

I had a look at adding the container/child address matching checks to Cadence: Just before performing a mutating operation on a container (array/map), we already transfer the child, so checking the invariant does not really help much: if we insert the assertion we can also check the transfer.

On the atree side, it's not easy to check this invariant: children are values at first, and getting the storable (which is needed to get the potential storage ID) is delayed until the very "end", e.g. deep down when the container/parent is not available anymore.

Maybe there isn't much we can do here

…omparator-and-hashinputprovider-to-mapiterator

fxamacker · 2023-10-03T22:02:12Z

@turbolent Thanks for detailed review and meeting notes!

I think everything discussed so far have been addressed in this PR. 🎉

More comments were added about how atree inlining works in commit a75e388
Commit 63ea7a7 handles values with outdated parent callbacks (value was removed or overwritten in parent container):
- the callback is no-op when callback detects value is no longer a child in its old parent.
- value unsets outdated parent callback
Commit 8d02bbc renamed existingValue.
Commit 042eb68 unlined removed or overwritten child (if it is inlined)
I double-checked that atree doesn't make assumption about value returned from StoredValue().
About "invariant that all values stored in an account always reference values in the same account"
- it isn't easy to check in code as you mentioned. However, we check this in tests, for example, atree.CheckStorageHealth() checks that all child or referenced slabs have the same address and atree validation functions also checks inlined slabs in memory have the same address as its parent.
I opened these issues to add more integration tests:
- Add more integration tests for inlining #346
- Add more integration tests for atree inlining cadence#2837

BTW, can you please take a look at PR #345? That is the 2nd of 2 atree inlining PRs.

turbolent

Fantastic work Faye! 👏

My review focused mainly on the changes to array and ordered map, I didn't review the tests, but they seem very thorough (18 kLOC wow!) 👏

As mentioned above, we can address remaining work in follow-up PRs

…ashinputprovider-to-mapiterator Add readonly iterators and support value mutations only from non-readonly iterators

fxamacker added enhancement New feature or request performance improvement breaking change changes to API that can break programs using Atree's API storage breaking change breaks existing stored data (requires storage migration) labels Sep 14, 2023

fxamacker requested review from ramtinms and turbolent as code owners September 14, 2023 17:35

fxamacker self-assigned this Sep 14, 2023

fxamacker marked this pull request as draft September 14, 2023 17:35

fxamacker changed the title ~~Atree Inlining (part 1 of 2): inline child arra and /map data slab into parent slab~~ Atree Inlining (part 1 of 2): inline child array and /map data slab into parent slab Sep 14, 2023

fxamacker changed the title ~~Atree Inlining (part 1 of 2): inline child array and /map data slab into parent slab~~ Atree Inlining (part 1 of 2): inline child array and map data slab into parent slab Sep 14, 2023

fxamacker changed the title ~~Atree Inlining (part 1 of 2): inline child array and map data slab into parent slab~~ Inline child array and map data slab into parent slab Sep 14, 2023

fxamacker marked this pull request as ready for review September 15, 2023 00:19

Check overwritten value in parentUpdater callback

6f25137

Currently, in parentUpdater callback, parent array/map resets same child value. Child value ID should match overwritten SlabIDStorable or Slab.SlabID(). This commit adds check to make sure same child value is being reset.

Avoid writing parent on update to uninlinable child

399684a

ramtinms approved these changes Sep 18, 2023

View reviewed changes

turbolent reviewed Sep 18, 2023

View reviewed changes

array.go Outdated Show resolved Hide resolved

array.go Show resolved Hide resolved

array.go Show resolved Hide resolved

array.go Outdated Show resolved Hide resolved

fxamacker added 2 commits September 19, 2023 11:17

Update comments about encoding format

c178a72

Update comments about encoding format

efc46e6

fxamacker commented Sep 19, 2023

View reviewed changes

typeinfo.go Outdated Show resolved Hide resolved

fxamacker added 7 commits September 20, 2023 13:41

Add comment to update ValueID when SlabID is changed

1643589

Use new ExtraData for inlined slab during decoding

72d3614

Decoded ExtraData (including TypeInfo) can be shared by all inlined slabs. This commit creates a new ExtraData with copied TypeInfo for inlined slabs to prevent accidental mutation.

Copy key for inlined composite during decoding

f66a85b

Decoded ExtraData (including keys) can be shared by all inlined composite referring to the same type. This commit copies key for inlined composite to prevent accidental mutation.

Add comments for safe use of range loop over Go map

f8e0992

fxamacker added 2 commits October 3, 2023 15:49

Rename variables for clarity

8d02bbc

Merge branch 'fxamacker/inline-array-and-map' into fxamacker/add-keyc…

5df8b16

…omparator-and-hashinputprovider-to-mapiterator

fxamacker mentioned this pull request Oct 3, 2023

Add more integration tests for atree inlining onflow/cadence#2837

Closed

turbolent approved these changes Oct 3, 2023

View reviewed changes

fxamacker added 4 commits October 3, 2023 17:51

Add more comments

2a6091a

Add MapIterator.CanMutate() predicate function

c07907d

Add ArrayIterator.CanMutate() predicate function

f671189

Merge pull request #345 from onflow/fxamacker/add-keycomparator-and-h…

5f1de0a

…ashinputprovider-to-mapiterator Add readonly iterators and support value mutations only from non-readonly iterators

fxamacker changed the base branch from main to feature/array-map-inlining October 4, 2023 16:09

fxamacker merged commit 0fc8f74 into feature/array-map-inlining Oct 4, 2023
11 checks passed

fxamacker changed the title ~~Inline child array and map data slab into parent slab~~ Atree Register Inlining and Data Deduplication Oct 6, 2023

This was referenced Oct 6, 2023

Deduplicate data when encoding nested Cadence composites to reduce memory and storage #347

Closed

Smoke tests need to check recently added data deduplication feature #349

Closed

Make smoke tests check recently added data deduplication feature #350

Merged

fxamacker mentioned this pull request Nov 2, 2023

Storage migration for Atree register inlining onflow/flow-go#4576

Closed

22 tasks

fxamacker mentioned this pull request Nov 22, 2023

Additional memory and storage savings are possible by deduplicating Cadence dictionary type info #358

Closed

fxamacker mentioned this pull request Dec 7, 2023

Atree storage migration onflow/flow-go#4633

Merged

This was referenced Jan 22, 2024

Optimize OrderedMap to reduce Flow's Mtrie size #296

Closed

Optimize Array to reduce Flow's Mtrie size and reduce number of reads #297

Closed

Improving efficiency when storing composite values (specially nested ones) onflow/cadence#1854

Closed

This was referenced Feb 15, 2024

Integrate Atree inlining and Cadence v0.42.x with flow-go onflow/flow-go#5296

Closed

Integrate Atree inlining and Cadence 1.0 with flow-go onflow/flow-go#5403

Closed

fxamacker mentioned this pull request Mar 13, 2024

Optimize atree to reduce Flow's mtrie size and reduce number of reads #292

Closed

fxamacker mentioned this pull request Sep 22, 2024

Atree Inlining & Deduplication with Cadence v1.0 onflow/flow-go#5554

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Atree Register Inlining and Data Deduplication #342

Atree Register Inlining and Data Deduplication #342

fxamacker commented Sep 14, 2023 •

edited

Loading

codecov-commenter commented Sep 17, 2023 •

edited

Loading

turbolent commented Sep 18, 2023

ramtinms left a comment

ramtinms Sep 18, 2023

ramtinms Sep 18, 2023

turbolent Sep 25, 2023

ramtinms Sep 18, 2023

fxamacker Sep 21, 2023

ramtinms Sep 18, 2023

fxamacker Sep 21, 2023

turbolent commented Oct 3, 2023

fxamacker commented Oct 3, 2023

turbolent left a comment •

edited

Loading

Atree Register Inlining and Data Deduplication #342

Atree Register Inlining and Data Deduplication #342

Conversation

fxamacker commented Sep 14, 2023 • edited Loading

Problem

Changes

TODO

In this PR

Atree Inlining Part 2

Cadence Integration

codecov-commenter commented Sep 17, 2023 • edited Loading

Codecov Report

turbolent commented Sep 18, 2023

ramtinms left a comment

Choose a reason for hiding this comment

ramtinms Sep 18, 2023

Choose a reason for hiding this comment

ramtinms Sep 18, 2023

Choose a reason for hiding this comment

turbolent Sep 25, 2023

Choose a reason for hiding this comment

ramtinms Sep 18, 2023

Choose a reason for hiding this comment

fxamacker Sep 21, 2023

Choose a reason for hiding this comment

ramtinms Sep 18, 2023

Choose a reason for hiding this comment

fxamacker Sep 21, 2023

Choose a reason for hiding this comment

turbolent commented Oct 3, 2023

fxamacker commented Oct 3, 2023

turbolent left a comment • edited Loading

Choose a reason for hiding this comment

fxamacker commented Sep 14, 2023 •

edited

Loading

codecov-commenter commented Sep 17, 2023 •

edited

Loading

turbolent left a comment •

edited

Loading