Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

performance: turn cache misses during assembly into cache hits during eval #4617

Merged
merged 4 commits into from
Nov 4, 2022

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 cce, a user, AlgoAxel and algorandskiy September 30, 2022 20:39
ledger/acctupdates.go Outdated Show resolved Hide resolved
ledger/acctupdates.go Outdated Show resolved Hide resolved
@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
@@ -94,6 +95,7 @@ var DefaultConfig = PpConfig{
RandomizeFee: false,
RandomizeAmt: false,
RandomizeDst: false,
MaxRandomDst: 200000,
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

ledger/ledger.go Outdated Show resolved Hide resolved
ledger/lruaccts.go Outdated Show resolved Hide resolved
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

ledger/ledger.go Outdated Show resolved Hide resolved
ledger/lruaccts.go Outdated Show resolved Hide resolved
@icorderi icorderi force-pushed the feat/same-round-cache branch from 0c2e549 to 974968e Compare November 4, 2022 16:24

au.baseAccounts.flushPendingWrites()
au.baseResources.flushPendingWrites()
au.baseKVs.flushPendingWrites()
Copy link
Contributor Author

@icorderi icorderi Nov 4, 2022

Choose a reason for hiding this comment

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

rebased on master to get the boxes feature and added a flush on the baseKVs LRU.

@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.

au.baseResources.flushPendingWrites()
au.baseKVs.flushPendingWrites()

au.accountsMu.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer defer?

@@ -1327,6 +1340,12 @@ func (au *accountUpdates) lookupResource(rnd basics.Round, addr basics.Address,
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"

@@ -1428,6 +1448,12 @@ func (au *accountUpdates) lookupWithoutRewards(rnd basics.Round, addr basics.Add
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)

@@ -117,5 +152,8 @@ func (m *lruAccounts) prune(newSize int) (removed int) {
m.accountsList.remove(back)
removed++
}

// 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

@@ -139,5 +174,8 @@ func (m *lruResources) prune(newSize int) (removed int) {
m.resourcesList.remove(back)
removed++
}

// 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