prefetcher: enable prefetcher for assets and apps #4352

algorandskiy · 2022-08-04T00:12:37Z

Summary

Enable prefetcher for asset and app transactions except the foreign fields.

Test Plan

Modified all prefetcher error tests to ensure all transaction groups returned by the prefetecher even in case errors.
Checked prefetcher_alignment_test that validates prefetcher's behavior againsy apply logic.

codecov · 2022-08-04T00:47:20Z

Codecov Report

Merging #4352 (6bdb584) into master (7e8ca90) will increase coverage by 0.06%.
The diff coverage is 67.64%.

@@            Coverage Diff             @@
##           master    #4352      +/-   ##
==========================================
+ Coverage   54.11%   54.18%   +0.06%     
==========================================
  Files         401      401              
  Lines       51549    51583      +34     
==========================================
+ Hits        27898    27952      +54     
+ Misses      21311    21282      -29     
- Partials     2340     2349       +9

Impacted Files	Coverage Δ
ledger/internal/eval.go	`67.15% <8.57%> (-0.69%)`	⬇️
ledger/internal/prefetcher/prefetcher.go	`95.51% <98.50%> (+27.32%)`	⬆️
ledger/roundlru.go	`90.56% <0.00%> (-5.67%)`	⬇️
network/wsPeer.go	`66.03% <0.00%> (-4.86%)`	⬇️
ledger/blockqueue.go	`85.63% <0.00%> (-2.88%)`	⬇️
crypto/merkletrie/trie.go	`66.42% <0.00%> (-2.19%)`	⬇️
crypto/merkletrie/node.go	`91.62% <0.00%> (-1.87%)`	⬇️
catchup/peerSelector.go	`98.95% <0.00%> (-1.05%)`	⬇️
catchup/service.go	`68.14% <0.00%> (-0.50%)`	⬇️
network/wsNetwork.go	`64.82% <0.00%> (ø)`
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jannotti

I don't know. I'm sure it's better than it was, but I can't follow everything. The whole prefetcher is quite something.

jannotti · 2022-08-04T01:35:06Z

ledger/internal/eval.go

-						base.assets[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetHolding{value: *lr.Resource.AssetHolding, exists: true}
-					} else {
-						base.assets[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetHolding{exists: false}
+			if txgroup.Err == nil {


Can this be flattened/simplified?

if !ok { break } if txGroup.Err != nil { // log it } else { // for loop }

jannotti · 2022-08-04T01:45:33Z

ledger/internal/eval.go

+						base.creators[creatable{cindex: lr.CreatableIndex, ctype: lr.CreatableType}] =
+							foundAddress{exists: false}


I would find all of these lines much more readable if creatable literals did not use field names in the literals. I don't know how much others agree, but the repetitive noise words makes it very hard for me to see the things that matter.

And the structs that have exists: false could just use {}

It's not a big deal, maybe others find this more explicit and readable.

I'd probably even make tiny functions to construct the objects that have exists: true so that you only see the object that's being put inside.. It would get inlined.

I agree it will require less LoC but explicit stuff all in one place imo looks better. This is probably the reason it was written this way

I was trying to figure out what you were commenting on, but realized I toggled on ignoring whitespace, so none of these lines appear as changed to me ...

Chipping in on the readability.

some of these lines are 200+ characters long, I actually had to go full screen on a 34' ultrawide for a side-by-side comparison.

I find the left hand side of the assignments a lot less readable than the right hand side.

Consider the following modification:

// somewhere earlier in the code.. var asset = ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)} // ..the code we are looking at if lr.Resource.AssetHolding != nil { base.assets[asset] = cachedAssetHolding{value: *lr.Resource.AssetHolding, exists: true} } else { base.assets[asset] = cachedAssetHolding{exists: false} }

The rationale to move the pk construction to its own variable is that it's the same pk.

My eyes kept going up/down , left/scroll/right/more-scroll and forth between the lines to see if the pks being constructed had any difference.

The first line can be mode a bit more readable too, but honestly this is something a code formatter should handle automatically than deal with it line by line during PRs.

var asset = ledgercore.AccountAsset{ Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex) }

As for the explicit struct construction on the right hand side, I also personally prefer the explicit construction.
A good syntax color scheme that separates types, fields, methods, consts goes a long way there to tell whats going on at a glance.

I think it's too bad that Go was an early adopter of "there is one true formatting style" (right down to using tabs vs spaces) but did not extend that to include any hard advice on line length.

fixed to improve readability

jannotti · 2022-08-04T01:51:14Z

ledger/internal/eval.go

+						if lr.Resource.AssetHolding != nil {
+							base.assets[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetHolding{value: *lr.Resource.AssetHolding, exists: true}
+						} else {
+							base.assets[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetHolding{exists: false}
+						}


For example, this entire thing could be one concise line

Suggested change

if lr.Resource.AssetHolding != nil {

base.assets[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetHolding{value: *lr.Resource.AssetHolding, exists: true}

} else {

base.assets[ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)}] = cachedAssetHolding{exists: false}

}

base.assets[ledgercore.AccountAsset{*lr.Address, basics.AssetIndex(lr.CreatableIndex)}] = newCAH(lr.Resource.AssetHolding)

with the help of newCAH that makes a cachedAssetHolding with exists = true or false based on whether the pointer argument it gets is nil.

ledger/internal/prefetcher/prefetcher.go

jannotti · 2022-08-04T02:19:26Z

ledger/internal/prefetcher/prefetcher.go

+				// do not preload Txn.ForeignApps, Txn.ForeignAssets, Txn.Accounts
+				// since they might be non-used arbitrary values


They could be, but it seems like we should try to load them anyway, and fail gracefully.

There's even a case to be made for load the cross product - every holding for each account/asset pair, and local state / app pair. But that seems like a lot. I don't know.

I have this funny idea that we should execute the transaction with a special ledger that responds only from cache, and fakes answers to things that aren't in cache, but records what was requested. And we use that to decide what to prefetch.

Yes, I agree, there should be a cache factory in ledger for Round X that is populated assembly time by data from X-1, and used in validation.
I think @cce added this into future improvements list for lowering round time

agreed, why not return to prefetching foreign arrays like we did before?

this might be abused, so I'd prefer not to.

ledger/internal/prefetcher/prefetcher.go

ledger/internal/eval.go

brianolson

LGTM

brianolson · 2022-09-14T16:05:05Z

ledger/internal/prefetcher/prefetcher.go

 			switch stxn.Txn.Type {
 			case protocol.PaymentTx:
 				loadAccountsAddAccountTask(&stxn.Txn.Receiver, task, accountTasks, queue)
 				loadAccountsAddAccountTask(&stxn.Txn.CloseRemainderTo, task, accountTasks, queue)
 			case protocol.AssetConfigTx:
+				loadAccountsAddResourceTask(nil, basics.CreatableIndex(stxn.Txn.ConfigAsset), basics.AssetCreatable, task, resourceTasks, queue)


This takes a cache miss on initial acfg creation when Txn.ConfigAsset == 0. We could know that id based on block header txn count and the position within the payset. Also probably not a big deal, pretty rare operation, rare cache miss.

There are two different things here worth thinking about, I'm not sure which one you mean. First, in an acfg with id=0, we should not try to prefetch the 0 ASA, since we know it doesn't exist. It looks like we handle that case in loadAccountsAddResource.

And then there's the question of whether block validation is slowed down because we didn't load the thing that will eventually be created. I don't think so. At evaluation time, acfg create should not be looking up the newly created id. (This brings up a silly optimization we should do, though it presumably does not happen in "good" txns - short-circuit the db lookup for creatables that can't exist because the id is too high.)

Also, need to be careful now about predicting the id based on position in payset. Inner txns count, so id is not just the block start id + payset index.

This takes a cache miss on initial acfg creation when Txn.ConfigAsset == 0. We could know that id based on block header txn count and the position within the payset.

since we know it doesn't exist. It looks like we handle that case in loadAccountsAddResource.

I guess we want to load creator's record since it will touched in apply. Maybe as a separate PR

icorderi

LGTM

icorderi · 2022-09-15T15:12:27Z

ledger/internal/eval.go

+						base.creators[creatable{cindex: lr.CreatableIndex, ctype: lr.CreatableType}] =
+							foundAddress{exists: false}


Chipping in on the readability.

some of these lines are 200+ characters long, I actually had to go full screen on a 34' ultrawide for a side-by-side comparison.

I find the left hand side of the assignments a lot less readable than the right hand side.

Consider the following modification:

// somewhere earlier in the code.. var asset = ledgercore.AccountAsset{Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex)} // ..the code we are looking at if lr.Resource.AssetHolding != nil { base.assets[asset] = cachedAssetHolding{value: *lr.Resource.AssetHolding, exists: true} } else { base.assets[asset] = cachedAssetHolding{exists: false} }

The rationale to move the pk construction to its own variable is that it's the same pk.

My eyes kept going up/down , left/scroll/right/more-scroll and forth between the lines to see if the pks being constructed had any difference.

The first line can be mode a bit more readable too, but honestly this is something a code formatter should handle automatically than deal with it line by line during PRs.

var asset = ledgercore.AccountAsset{ Address: *lr.Address, Asset: basics.AssetIndex(lr.CreatableIndex) }

As for the explicit struct construction on the right hand side, I also personally prefer the explicit construction.
A good syntax color scheme that separates types, fields, methods, consts goes a long way there to tell whats going on at a glance.

algorandskiy

Also prevented prefetching sender/receiver for zero transfers.

algorandskiy · 2022-09-15T19:55:36Z

ledger/internal/prefetcher/prefetcher.go

 			switch stxn.Txn.Type {
 			case protocol.PaymentTx:
 				loadAccountsAddAccountTask(&stxn.Txn.Receiver, task, accountTasks, queue)
 				loadAccountsAddAccountTask(&stxn.Txn.CloseRemainderTo, task, accountTasks, queue)
 			case protocol.AssetConfigTx:
+				loadAccountsAddResourceTask(nil, basics.CreatableIndex(stxn.Txn.ConfigAsset), basics.AssetCreatable, task, resourceTasks, queue)


This takes a cache miss on initial acfg creation when Txn.ConfigAsset == 0. We could know that id based on block header txn count and the position within the payset.

since we know it doesn't exist. It looks like we handle that case in loadAccountsAddResource.

I guess we want to load creator's record since it will touched in apply. Maybe as a separate PR

algorandskiy · 2022-09-15T19:56:06Z

ledger/internal/eval.go

+						base.creators[creatable{cindex: lr.CreatableIndex, ctype: lr.CreatableType}] =
+							foundAddress{exists: false}


fixed to improve readability

algorandskiy

also added a missed TestEvaluatorPrefetcherAlignmentAssetTransfer test

algorandskiy · 2022-09-15T20:41:04Z

ops, broke some asset receiver prefetching, fixing

algorandskiy · 2022-09-16T16:22:53Z

Fixed one unit test and adjusted AssetReceiver prefetching condition.

algonautshant · 2022-09-19T20:19:18Z

ledger/internal/prefetcher/prefetcher.go

+				}
+				if !stxn.Txn.AssetReceiver.IsZero() {
+					if stxn.Txn.AssetAmount != 0 || (stxn.Txn.AssetReceiver == stxn.Txn.Sender) {
+						// if not zero transfer and not not opt in


Should this comment be updated? It seems to have the opt in here as well.

Suggested change

// if not zero transfer and not not opt in

// if not zero transfer or is opt in

the condition is
if stxn.Txn.AssetAmount != 0 or (stxn.Txn.AssetAmount == 0 || (stxn.Txn.AssetReceiver == stxn.Txn.Sender))
i.e. not zero amount or optin. That is what the comment says

algonautshant

LGTM

algorandskiy · 2022-10-24T21:08:32Z

Thank you for review! I'll merge it right after the boxes go through to prevent possible conflicts.

algorandskiy added Enhancement Team Carbon-11 labels Aug 4, 2022

algorandskiy requested review from cce, jannotti and algonautshant August 4, 2022 00:12

algorandskiy self-assigned this Aug 4, 2022

jannotti previously approved these changes Aug 4, 2022

View reviewed changes

algorandskiy commented Aug 12, 2022

View reviewed changes

ledger/internal/eval.go Outdated Show resolved Hide resolved

algorandskiy force-pushed the pavel/prefetch branch from f8bd960 to b78b539 Compare August 12, 2022 19:43

michaeldiamant dismissed jannotti’s stale review via b78b539 August 16, 2022 19:11

brianolson reviewed Sep 14, 2022

View reviewed changes

icorderi previously approved these changes Sep 15, 2022

View reviewed changes

algorandskiy and others added 6 commits September 15, 2022 15:35

wip: do not break prefetching on error

a2902e8

Add a test for a app transaction with the offending foreign asset field

d050b61

Re-enable some prefetcher tests

7095d5a

CR: update comment

7d1ca4d

CR: readability

0e1ee74

Do not prefetch zero asset transfers

76b71c9

algorandskiy dismissed icorderi’s stale review via 76b71c9 September 15, 2022 20:09

algorandskiy force-pushed the pavel/prefetch branch from b78b539 to 76b71c9 Compare September 15, 2022 20:09

algorandskiy commented Sep 15, 2022

View reviewed changes

algorandskiy requested review from jannotti, icorderi and brianolson September 15, 2022 20:09

algorandskiy commented Sep 15, 2022

View reviewed changes

icorderi previously approved these changes Sep 15, 2022

View reviewed changes

Fix unit test

ee15451

algorandskiy dismissed icorderi’s stale review via ee15451 September 16, 2022 16:19

algorandskiy requested a review from icorderi September 16, 2022 17:43

algonautshant reviewed Sep 20, 2022

View reviewed changes

algonautshant previously approved these changes Sep 20, 2022

View reviewed changes

algorandskiy dismissed algonautshant’s stale review via 4106912 September 20, 2022 14:23

CR feedback

6bdb584

algorandskiy force-pushed the pavel/prefetch branch from 4106912 to 6bdb584 Compare September 20, 2022 14:34

algorandskiy requested a review from algonautshant October 11, 2022 19:06

algonautshant approved these changes Oct 13, 2022

View reviewed changes

brianolson approved these changes Oct 24, 2022

View reviewed changes

algorandskiy merged commit e6f396b into algorand:master Oct 31, 2022

This was referenced Nov 1, 2022

test: fix after boxes and prefetcher remerge #4716

Merged

prefetcher test #4250

Closed

michaeldiamant mentioned this pull request Nov 1, 2022

Boxes: Add prefetch support #4718

Open

Algo-devops-service mentioned this pull request Nov 1, 2022

go-algorand 3.12.0-beta Release PR #4730

Merged

Algo-devops-service mentioned this pull request Nov 10, 2022

go-algorand 3.12.0-stable Release PR #4783

Closed

Algo-devops-service mentioned this pull request Nov 28, 2022

go-algorand 3.12.2-stable Release PR #4837

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prefetcher: enable prefetcher for assets and apps #4352

prefetcher: enable prefetcher for assets and apps #4352

algorandskiy commented Aug 4, 2022

codecov bot commented Aug 4, 2022 •

edited

Loading

jannotti left a comment

jannotti Aug 4, 2022

jannotti Aug 4, 2022

algorandskiy Aug 4, 2022

cce Aug 24, 2022

icorderi Sep 15, 2022

jannotti Sep 15, 2022

algorandskiy Sep 15, 2022

jannotti Aug 4, 2022

jannotti Aug 4, 2022

algorandskiy Aug 4, 2022

cce Sep 20, 2022

algorandskiy Sep 20, 2022

brianolson left a comment

brianolson Sep 14, 2022

jannotti Sep 15, 2022

algorandskiy Sep 15, 2022

icorderi left a comment

icorderi Sep 15, 2022

algorandskiy left a comment

algorandskiy Sep 15, 2022

algorandskiy Sep 15, 2022

algorandskiy left a comment

algorandskiy commented Sep 15, 2022

algorandskiy commented Sep 16, 2022

algonautshant Sep 19, 2022

algorandskiy Sep 20, 2022

algonautshant left a comment

algorandskiy commented Oct 24, 2022

		base.creators[creatable{cindex: lr.CreatableIndex, ctype: lr.CreatableType}] =
		foundAddress{exists: false}

		// do not preload Txn.ForeignApps, Txn.ForeignAssets, Txn.Accounts
		// since they might be non-used arbitrary values

	// if not zero transfer and not not opt in
	// if not zero transfer or is opt in

prefetcher: enable prefetcher for assets and apps #4352

prefetcher: enable prefetcher for assets and apps #4352

Conversation

algorandskiy commented Aug 4, 2022

Summary

Test Plan

codecov bot commented Aug 4, 2022 • edited Loading

Codecov Report

jannotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianolson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icorderi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algorandskiy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algorandskiy left a comment

Choose a reason for hiding this comment

algorandskiy commented Sep 15, 2022

algorandskiy commented Sep 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algonautshant left a comment

Choose a reason for hiding this comment

algorandskiy commented Oct 24, 2022

codecov bot commented Aug 4, 2022 •

edited

Loading