onflow / atree

Atree provides scalable arrays and scalable ordered maps.
https://onflow.org
Apache License 2.0
40 stars 16 forks source link

Use encoded type info to deduplicate extra data #381

Closed fxamacker closed 6 months ago

fxamacker commented 6 months ago

This handles edge case for Cadence type.ID() returning same ID for different types.

Currently, we use TypeInfo.Identifier() to deduplicate extra data and type info. However, TypeInfo.Identifier() is implemented in another package and we can't enforce its uniqueness for different types. If TypeInfo.Identifier() returns same ID for different types, different types is wrongly deduplicated.

This PR uses encoded type info via TypeInfo.Encode() to deduplicate extra data. This prevents differently encoded type info from being deduplicated by mistake.

This PR also uses sync.Pool to reuse buffer for type info encoding.

This problem was found by using latest testnet data for atree migration + Cadence v0.42. Prior tests using mainnet data did not encounter this issue.


codecov-commenter commented 6 months ago

Codecov Report

Attention: Patch coverage is 78.64078% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 63.10%. Comparing base (92714ca) to head (83c99b3).

Files Patch % Lines
typeinfo.go 85.71% 9 Missing and 4 partials :warning:
map.go 25.00% 4 Missing and 2 partials :warning:
array.go 25.00% 2 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## feature/array-map-inlining #381 +/- ## ============================================================== - Coverage 63.18% 63.10% -0.08% ============================================================== Files 15 15 Lines 10955 10983 +28 ============================================================== + Hits 6922 6931 +9 - Misses 3054 3064 +10 - Partials 979 988 +9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fxamacker commented 6 months ago

@turbolent @ramtinms PTAL :pray:

With this PR, full atree migration with Cadence 0.42 + atree inlining/deduplication + validation passed yesterday using

BTW, to avoid blocking team during 4+ hour test runs, I created and am using m1-ultramem-160 vm in central region.

turbolent commented 6 months ago

@fxamacker That's really great news, well done! 👏 🎉

Good idea to spin up a separate machine to avoid blocking 👍