Preload asset/app creators before evaluation

[tolikzinovyev]

Summary

Preloading asset/app creators in a batch improves performance by ~10%.

Depends on #746.

[gmalouf]

Suggest holding off for now - build is failing, and the snapshotting and beefier machine changes will play a much larger role in performance improvement.

[codecov-commenter]

Codecov Report

Merging #749 (134e2f7) into develop (2b71eeb) will increase coverage by 0.19%.
The diff coverage is 78.94%.

@@             Coverage Diff             @@
##           develop     #749      +/-   ##
===========================================
+ Coverage    55.17%   55.37%   +0.19%     
===========================================
  Files           27       27              
  Lines         3815     3850      +35     
===========================================
+ Hits          2105     2132      +27     
- Misses        1435     1440       +5     
- Partials       275      278       +3     

Impacted Files	Coverage Δ
idb/postgres/postgres.go	48.47% <78.94%> (+0.73%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b71eeb...134e2f7. Read the comment docs.

[winder]

This looks correct, but should it be part of the EvalForIndexer interface? I thought that interface's main purpose was to make it easier to preload everything.

[tolikzinovyev]

This looks correct, but should it be part of the EvalForIndexer interface? I thought that interface's main purpose was to make it easier to preload everything.

I don't think I understand.

[winder]

This looks correct, but should it be part of the EvalForIndexer interface? I thought that interface's main purpose was to make it easier to preload everything.

I don't think I understand.

I'm asking about the design here, not the implementation.

You're using a combination of these new methods:

	LookupWithoutRewards(map[basics.Address]struct{}) (map[basics.Address]*basics.AccountData, error)
	GetAssetCreator(map[basics.AssetIndex]struct{}) (map[basics.AssetIndex]FoundAddress, error)
	GetAppCreator(map[basics.AppIndex]struct{}) (map[basics.AppIndex]FoundAddress, error)

I thought one reason for those methods was to avoid needing this sort of preloading, so I wonder if the interface is wrong.

Should it be a single method instead of 3? Or perhaps we add a 4th method to the interface specifically designed to help with caching.

[tolikzinovyev]

If your suggestion is to have the code that decides what to preload in go-algorand, then @tsachiherman was against it. We still need the interface functions in case we forget to preload something and the evaluator needs it. Let me know if this doesn't quite answer your questions.

[tsachiherman]

@winder - there are two "faces" for the evaluator:
We try to preload all the required resources before the evaluation. However, if during evaluation we find that we need additional resources, we need to provide the functionality to retrieve them.

The reason the evaluator need to be "dynamic" is that it's also being used for the transaction pool, where we add transactions on the fly. I wouldn't oppose to open a discussion on whether the evaluator need to be resource-predictable or not ( outside the scope of this PR ).

[winder]

@tolikzinovyev @tsachiherman thanks for the explanations. I guess the difference between algod and indexer is that algod's caching system exists outside of these functions and Indexer's is using them to initialize the cache.

It might be good to rename this function to something like preloadEvalResources to be more specific about what sort of preparation is happening.

Do we need to modify GetAssetCreator and GetAppCreator to check the cache? Wouldn't the evaluator call these functions a second time after preloading everything?

[tolikzinovyev]

After we give the resources to the evaluator, it's not supposed to request them.