Add ledger.GetBlockAddresses() #2872

tolikzinovyev · 2021-09-11T04:32:05Z

Summary

Indexer needs to know all accounts referenced in a block to be able to read all of them in one batch. This PR creates a function that returns all such account addresses.

codecov-commenter · 2021-09-11T05:10:12Z

Codecov Report

Merging #2872 (3831123) into master (83fb239) will decrease coverage by 0.03%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##           master    #2872      +/-   ##
==========================================
- Coverage   47.37%   47.33%   -0.04%     
==========================================
  Files         351      351              
  Lines       56523    56529       +6     
==========================================
- Hits        26776    26760      -16     
- Misses      26743    26761      +18     
- Partials     3004     3008       +4

Impacted Files	Coverage Δ
ledger/eval.go	`76.17% <62.50%> (-0.66%)`	⬇️
ledger/roundlru.go	`90.56% <0.00%> (-5.67%)`	⬇️
ledger/blockqueue.go	`82.18% <0.00%> (-2.88%)`	⬇️
network/requestTracker.go	`70.25% <0.00%> (-1.30%)`	⬇️
catchup/peerSelector.go	`98.95% <0.00%> (-1.05%)`	⬇️
cmd/tealdbg/debugger.go	`72.86% <0.00%> (-1.01%)`	⬇️
ledger/acctupdates.go	`62.13% <0.00%> (-0.51%)`	⬇️
network/wsNetwork.go	`60.90% <0.00%> (-0.19%)`	⬇️
catchup/service.go	`69.35% <0.00%> (ø)`
network/wsPeer.go	`75.20% <0.00%> (+0.55%)`	⬆️
... and 2 more

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 83fb239...3831123. Read the comment docs.

tsachiherman

The change looks good - but they don't perform that great. I tested your changes using

go test -v . --run XX --bench BenchmarkBlockEvaluatorRAMNoCrypto -count=10 | grep BenchmarkBlockEvaluatorRAMNoCrypto

and received the following performance metrics on master:

BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 945332700 ns/op	     18907 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 887004810 ns/op	     17741 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 898731697 ns/op	     17976 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 905146646 ns/op	     18104 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 895749462 ns/op	     17916 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 876699177 ns/op	     17535 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 889700168 ns/op	     17795 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 888446000 ns/op	     17770 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 888201852 ns/op	     17765 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 916630812 ns/op	     18333 ns/eval_validate_tx

compared to the ones from your branch:

BenchmarkBlockEvaluatorRAMNoCrypto-8   	       1	1016715840 ns/op	     20335 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       1	1026562100 ns/op	     20533 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	1005568624 ns/op	     20112 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       1	1008136765 ns/op	     20164 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       1	1016944071 ns/op	     20340 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	1016153946 ns/op	     20324 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 974119522 ns/op	     19483 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 957210924 ns/op	     19145 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 879668276 ns/op	     17594 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 913933502 ns/op	     18279 ns/eval_validate_tx

In the vast of the cases, master perform better compared to your branch. I would suggest that you'll keep the existing implementation of loadAccounts as is, and just have your own changes ( you might want to add some comments that it's a code duplication that need to be converged ).

For the purposes of the indexer, we can optimize that code on another task - but there is no reason to introduce a regression to the master.

winder · 2021-09-13T15:11:49Z

Would it make sense to add this as a function to SignedTxnInBlock instead of as part of the evaluator?

tsachiherman · 2021-09-13T15:16:08Z

Would it make sense to add this as a function to SignedTxnInBlock instead of as part of the evaluator?

I would be ok if we were to add the method to the transactions package. But why SignedTxnInBlock and not Transaction ?

winder · 2021-09-13T15:46:28Z

Would it make sense to add this as a function to SignedTxnInBlock instead of as part of the evaluator?

I would be ok if we were to add the method to the transactions package. But why SignedTxnInBlock and not Transaction ?

Transaction would be better. I was thinking we needed the ApplyData in order to figure out who participates in the inner transactions, but txn.ApplicationCallTxnFields.Accounts is available for that.

Do we always need all of these accounts available? I suppose the TEAL evaluation needs to read accounts regardless of whether they are modified...

If all of these accounts are added to the txn participation table, this is going to change the result of a txns by account lookup. We may want to add this to the release notes, and possibly add a new address role for it.

tolikzinovyev · 2021-09-13T16:48:37Z

I can't reproduce the difference in performance (perhaps because I'm using the newest version of go). I changed the code slightly. Could you run the benchmark again?

tsachiherman · 2021-09-13T17:41:11Z

I can't reproduce the difference in performance (perhaps because I'm using the newest version of go). I changed the code slightly. Could you run the benchmark again?

I'm using

go version go1.16.7 darwin/amd64

and running the same benchmark again:

BenchmarkBlockEvaluatorRAMNoCrypto
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 937884934 ns/op	     18758 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	1104762888 ns/op	     22097 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       1	1051855114 ns/op	     21039 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 945192180 ns/op	     18905 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 964409754 ns/op	     19289 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 896160418 ns/op	     17924 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 945274962 ns/op	     18906 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 971673884 ns/op	     19435 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       1	1020561048 ns/op	     20412 ns/eval_validate_tx
BenchmarkBlockEvaluatorRAMNoCrypto-8   	       2	 971733400 ns/op	     19436 ns/eval_validate_tx

is slightly better, but not there yet.

tolikzinovyev · 2021-09-13T17:57:09Z

It might be something to do with the new optimization in go 1.17. https://golang.org/doc/go1.17#compiler

How significant is this benchmark anyway? The cost of eval should be dominated by crypto and disk.

tsachiherman · 2021-09-13T18:02:49Z

There is no reason to have any performance loss for adding new, non-interfering functionality. I'm sure that the performance loss would have been much more noticeable if we had a benchmark for loadAccounts - but we don't.

Given that refactoring this code isn't really what you're after, just rollback the changes in loadAccounts and it will be good to go. Once we move to go 1.17, I'll be more than happy to re-test that.

tsachiherman · 2021-09-13T19:05:09Z

ledger/eval.go

+	*out = append(
+		*out, txn.Sender, txn.Receiver, txn.CloseRemainderTo, txn.AssetSender,
+		txn.AssetReceiver, txn.AssetCloseTo, txn.FreezeAccount)
+	*out = append(txn.ApplicationCallTxnFields.Accounts)


I think that you meant

*out = append(*out, txn.ApplicationCallTxnFields.Accounts...)

tsachiherman

looks good to me.

Add ledger.GetBlockAddresses().

3831123

tolikzinovyev added the Team Carbon-11 label Sep 11, 2021

tolikzinovyev self-assigned this Sep 11, 2021

tolikzinovyev requested a review from tsachiherman September 11, 2021 04:32

tsachiherman suggested changes Sep 11, 2021

View reviewed changes

Avoid memory allocations.

0448537

tolikzinovyev added 2 commits September 13, 2021 14:38

Revert the changes in loadAccounts().

714d557

Fix review god warning.

480afb2

tsachiherman reviewed Sep 13, 2021

View reviewed changes

fix

8dd4a33

tsachiherman approved these changes Sep 13, 2021

View reviewed changes

tsachiherman merged commit d75f4eb into algorand:master Sep 13, 2021

tolikzinovyev deleted the indexer-batch branch September 13, 2021 20:32

onetechnical mentioned this pull request Sep 15, 2021

go-algorand 3.0.0-beta #2893

Merged

algojack mentioned this pull request Sep 24, 2021

go-algorand 3.0.1-stable #2949

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ledger.GetBlockAddresses() #2872

Add ledger.GetBlockAddresses() #2872

tolikzinovyev commented Sep 11, 2021

codecov-commenter commented Sep 11, 2021 •

edited

Loading

tsachiherman left a comment

winder commented Sep 13, 2021

tsachiherman commented Sep 13, 2021

winder commented Sep 13, 2021

tolikzinovyev commented Sep 13, 2021

tsachiherman commented Sep 13, 2021

tolikzinovyev commented Sep 13, 2021

tsachiherman commented Sep 13, 2021

tsachiherman Sep 13, 2021

tolikzinovyev Sep 13, 2021

tsachiherman left a comment

Add ledger.GetBlockAddresses() #2872

Add ledger.GetBlockAddresses() #2872

Conversation

tolikzinovyev commented Sep 11, 2021

Summary

codecov-commenter commented Sep 11, 2021 • edited Loading

Codecov Report

tsachiherman left a comment

Choose a reason for hiding this comment

winder commented Sep 13, 2021

tsachiherman commented Sep 13, 2021

winder commented Sep 13, 2021

tolikzinovyev commented Sep 13, 2021

tsachiherman commented Sep 13, 2021

tolikzinovyev commented Sep 13, 2021

tsachiherman commented Sep 13, 2021

tsachiherman Sep 13, 2021

Choose a reason for hiding this comment

tolikzinovyev Sep 13, 2021

Choose a reason for hiding this comment

tsachiherman left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 11, 2021 •

edited

Loading