Conversation
Almost all comments, except the use of "clear"
Still needs tests for absent boxes
We can do a more precise job of prefetch than we have previously, especially with tx.Access resources.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6555 +/- ##
===========================================
- Coverage 63.60% 47.91% -15.69%
===========================================
Files 486 646 +160
Lines 67806 88326 +20520
===========================================
- Hits 43125 42321 -804
- Misses 21188 43215 +22027
+ Partials 3493 2790 -703
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
algorandskiy
left a comment
There was a problem hiding this comment.
I wish boxes/foreign apps were added and refactored as a consequent PR (or another way around) to simplify the review.
algorandskiy
left a comment
There was a problem hiding this comment.
The code looks correct.
Requesting one fix here
app = stxn.Txn.ForeignApps[br.Index-1]
|
Ran through an LLM locally, found a potential sizing bug in the prefetch queue capacity math; I think allocating more space as in the first suggested fix probably is the way to handle this:
Suggested fix direction
|
Agreed, excellent catch, Gemini. 1 is easy and I'll do that unless 2 turns out to be relatively easy too. I prefer 2, since it's futureproof, and doesn't waste space. |
|
I did it the "hard" way, but I think it's much nicer to not care about a max number of resources per group. I ran benchmarks to make sure there was no regression |
c7d6611 to
c4e81e8
Compare
c4e81e8 to
2fa87f1
Compare
algorandskiy
left a comment
There was a problem hiding this comment.
I like the refactoring, incl used vs 'len/cap` thing - one less thing to track, one less bug
Co-authored-by: cce <51567+cce@users.noreply.github.com>
Co-authored-by: cce <51567+cce@users.noreply.github.com>
Co-authored-by: cce <51567+cce@users.noreply.github.com>
Summary
This PR improves prefetching in several ways:
ForeignApps. Previously we did not fetch anything fromAccounts,ForeignApps, andForeignAssetsbecause it's not really known if those resources will be fetched at evaluation time or, for example, a holding made available by an account and an asset would be fetched, but not the account and asset params. Upon further consideration, it seems very likely that any included app is fetched. The only other reason to include it would be to fetch locals for that app and one of the accounts. But locals are less commonly used these days, global access or invoking the app in an inner txn seem much more likely. So we prefetch.Test Plan