Skip to content

Conversation

@icorderi
Copy link
Contributor

@icorderi icorderi commented Sep 30, 2022

Summary

Scenario:

1. Node accepts txns into tx pool
2. Node receives a proposal with the same txns and validates it

At 1, accounts are read and saved to LRU as pending
At 2, prefetcher loads accounts. Need to ensure LRU is hit.

The problem statement applies both to accounts and resources.

Additionally, we taught the LRU to track accounts and resources that were not found in the DB.
Thus avoiding repeated DB queries on objects that do not exist.

Test Plan

  • Pass existing tests.
  • Run load scenarios and compare performance.

Benchmark on scenario-1s

We run the scenario-1s with the default script and using a random destination (max200k) both on master and this branch.
The transactions per second (TPS) results are shown in figure 1.

Page2

figure 1: TPS comparison between this branch and master.

@CLAassistant
Copy link

CLAassistant commented Sep 30, 2022

CLA assistant check
All committers have signed the CLA.

@icorderi icorderi requested review from a user, AlgoAxel, algorandskiy and cce September 30, 2022 20:39
@icorderi icorderi force-pushed the feat/same-round-cache branch 2 times, most recently from 988803e to 93a1624 Compare October 5, 2022 19:40
@codecov
Copy link

codecov bot commented Oct 5, 2022

Codecov Report

Merging #4617 (0c2bc3a) into master (ae443cd) will decrease coverage by 0.18%.
The diff coverage is n/a.

❗ Current head 0c2bc3a differs from pull request most recent head 974968e. Consider uploading reports for the commit 974968e to get more accurate results

@@            Coverage Diff             @@
##           master    #4617      +/-   ##
==========================================
- Coverage   54.55%   54.37%   -0.19%     
==========================================
  Files         414      407       -7     
  Lines       53607    52389    -1218     
==========================================
- Hits        29244    28485     -759     
+ Misses      21935    21525     -410     
+ Partials     2428     2379      -49     
Impacted Files Coverage Δ
gen/generate.go 14.09% <0.00%> (-51.03%) ⬇️
ledger/internal/prefetcher/prefetcher.go 68.18% <0.00%> (-27.33%) ⬇️
data/txHandler.go 11.11% <0.00%> (-14.82%) ⬇️
data/ledger.go 31.38% <0.00%> (-13.87%) ⬇️
ledger/ledgercore/statedelta.go 7.32% <0.00%> (-6.71%) ⬇️
agreement/persistence.go 70.06% <0.00%> (-5.89%) ⬇️
cmd/goal/application.go 12.64% <0.00%> (-5.15%) ⬇️
cmd/algoh/blockstats.go 86.36% <0.00%> (-4.95%) ⬇️
ledger/ledger.go 67.30% <0.00%> (-3.14%) ⬇️
network/wsPeer.go 66.50% <0.00%> (-2.43%) ⬇️
... and 84 more

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

@icorderi icorderi force-pushed the feat/same-round-cache branch from 93a1624 to 02dccd7 Compare October 24, 2022 16:44
@icorderi icorderi force-pushed the feat/same-round-cache branch 3 times, most recently from dcd4447 to 0c2e549 Compare October 24, 2022 19:01
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change default value might change nightly results. @egieseke @algobarb please be prepared

Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix two remarks and this is good to go

@icorderi icorderi force-pushed the feat/same-round-cache branch from 0c2e549 to 974968e Compare November 4, 2022 16:24
@algorandskiy algorandskiy merged commit 207f964 into algorand:master Nov 4, 2022
Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this was already merged, but wanted to note a couple thnigs.

Biggest comment is on code that isn't here. Although boxes were rebased into this, I don't see the analogous changes for lrukv.go.

return macct.AccountResource(), rnd, nil
}

// check baseAccoiunts again to see if it does not exist
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"baseResources"

return macct.accountData.GetLedgerCoreAccountData(), rnd, rewardsVersion, rewardsLevel, nil
}

// check baseAccoiunts again to see if it does not exist
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"baseAccounts" (sp)

}

// clear the notFound list
m.notFound = make(map[basics.Address]struct{}, len(m.notFound))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supposedly, deleting in a loop over all keys is recognized by the compiler and optimized to be cheaper than reallocating. @cce posted something about it. https://go-review.googlesource.com/c/go/+/110055

}

// clear the notFound list
m.notFound = make(map[accountCreatable]struct{}, len(m.notFound))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@icorderi icorderi deleted the feat/same-round-cache branch December 16, 2022 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants