-
Notifications
You must be signed in to change notification settings - Fork 139
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
Invalidate references to nested resources upon the move of the outer resource #2460
Invalidate references to nested resources upon the move of the outer resource #2460
Conversation
I'll review this on Monday |
…dence into supun/resource-reference-invalidation
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:feature/stable-cadence commit 07f5125 Collapsed results for better readability
|
Codecov Report
@@ Coverage Diff @@
## feature/stable-cadence #2460 +/- ##
==========================================================
+ Coverage 79.39% 79.40% +0.01%
==========================================================
Files 336 336
Lines 80145 80137 -8
==========================================================
+ Hits 63628 63634 +6
+ Misses 14215 14207 -8
+ Partials 2302 2296 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@turbolent can you maybe have a look when you got some time? 🙏 |
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 catch and work @SupunS! Sorry I missed this
Do you think there is maybe a way to implement this without having to traverse the whole object graph? The traversal can get quickly very expensive
Yeah, I agree. While traversing whole object graph, additional register reads may be needed if they are not loaded already. |
onflow/atree#311 is ready now. Next step is to update to atree version which includes feature and using new iteration function |
…dence into supun/resource-reference-invalidation
Updated to use the new atree APIs: f21739f |
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.
Nice!
@SupunS Could you please add a benchmark that exercises this a bit? The current benchmarks don't, and it would be good to know what kind of performance impact the reference invalidation will have |
…dence into supun/resource-reference-invalidation
Added a benchmark in e90ddca. The results are as follows: Master:
Stable-Cadence (current):
This PR:
This test assumes the worst-case: i.e. All elements are loaded onto the memory (of an array of size 1000). On a side note, I noticed that when the resource array is loaded from storage, all elements seem to be also 'loaded' every time (so they get iterated every time, during resource tracking). Not sure if it is due to the mocked 'test ledger' we are using for unit tests, or the expected behavior. cc: @fxamacker |
Thank you for adding that benchmark @SupunS! I think the worst-case decrease is acceptable, if it becomes an issue we can look into optimizing it.
Hmm, that seems odd. Could you please open a separate issue (bug?) for that? We should look into it, normally this should not happen. It doesn't block this PR. |
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!
Closes #2458
Description
When a resource is moved, invalidate references taken not only to the moved resource, but also the references taken to the nested resources of that moved resource.
Also refactored the code to move all the invalidation logic to a single place, which was scattered in the Transfer/Destroy methods of each value type.
master
branchFiles changed
in the Github PR explorer