Cache results of static types migration and entitlements migration #3396
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
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:master commit bf9965d Collapsed results for better readability
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
dsainati1
approved these changes
Jun 5, 2024
SupunS
approved these changes
Jun 5, 2024
Yeah, that's a good point! I think the encoding is fairly similar to the type ID construction (in orders of magnitude), but slightly more expensive computation and memory. I can imagine that for many simple cases in the static type migration the overhead of the caching might be larger than just running the migration. Though for the entitlements migration the caching will likely be worth it. I'll put the two caches behind two separate flags. |
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.
Work towards #3297
Description
In #3375 we removed the broken caching of results of the static types migration and entitlements migration.
Bring back the caching, but use the encoding of the static type instead of the type ID. The encoding of a static type isn't ambiguous or lacking any details compared to the type ID. I double checked that legacy / pre-1.0 information in static types, like the legacy restricted type in an intersection type (previously restriction type), is not only decoded properly for the migration, but also encoded again with the legacy information (which isn't the case for type IDs).
Disable the caching by default, so the default behaviour is always correct. In flow-go, this feature will be enabled through a flag (again, off by default).
I tested this approach in flow-go, and a run with caching always produces the same new state commitment as a non-caching run.
masterbranchFiles changedin the Github PR explorer