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

ledger: Remove redundant block header cache #5540

Merged

Conversation

algonautshant
Copy link
Contributor

@algonautshant algonautshant commented Jul 11, 2023

This PR removes headerCache blockHeaderCache from Ledger struct.
All requests to BlockHdr function which were fulfilled by headerCache are now addressed by l.txTail.blockHeader, and fallback, when the txTail does not have the round, to l.blockQ.getBlockHdr.

All instances of BlockHdrCached are eliminated or replaced by BlockHdr.

Impact on the locks:

  • The locking scheme had to change to make this possible. Mainly, l.trackerMu will no longer protect the txTail, instead, txTail mutex will be responsible for protecting the txTail actions.
  • BlockHdrCached function will no longer lock l.trackerMu.
  • trackerRegistry initialize will be guarded by the tr.mu lock.

Resolves: https://github.com/algorand/go-algorand-internal/issues/2147

#5566 will address the test failure since it is independent of this work.

ledger/ledger.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #5540 (77c3b4c) into master (d82200a) will decrease coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5540      +/-   ##
==========================================
- Coverage   55.06%   54.96%   -0.10%     
==========================================
  Files         465      463       -2     
  Lines       64584    64468     -116     
==========================================
- Hits        35561    35437     -124     
- Misses      26652    26659       +7     
- Partials     2371     2372       +1     
Files Changed Coverage Δ
cmd/tealdbg/localLedger.go 66.84% <ø> (+0.71%) ⬆️
daemon/algod/api/server/v2/dryrun.go 72.70% <ø> (+0.39%) ⬆️
ledger/eval/applications.go 38.63% <ø> (+0.43%) ⬆️
ledger/eval/cow.go 77.57% <ø> (+0.92%) ⬆️
ledger/eval/eval.go 51.04% <ø> (+0.11%) ⬆️
ledger/evalindexer.go 51.04% <ø> (+1.04%) ⬆️
data/transactions/logic/eval.go 92.18% <100.00%> (+0.05%) ⬆️
ledger/acctupdates.go 70.73% <100.00%> (-0.07%) ⬇️
ledger/ledger.go 69.47% <100.00%> (-1.09%) ⬇️
ledger/tracker.go 74.74% <100.00%> (-1.22%) ⬇️
... and 1 more

... and 13 files with indirect coverage changes

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

ledger/acctupdates.go Outdated Show resolved Hide resolved
@algonautshant algonautshant marked this pull request as ready for review July 13, 2023 20:34
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 remove exceed locks from txtal.commitRound

ledger/txtail.go Outdated Show resolved Hide resolved
ledger/txtail.go Outdated Show resolved Hide resolved
ledger/ledger.go Show resolved Hide resolved
algorandskiy
algorandskiy previously approved these changes Jul 14, 2023
@algorandskiy
Copy link
Contributor

Looking forward for part 2 with BlockHdrCached removal

Copy link
Contributor

@iansuvak iansuvak left a comment

Choose a reason for hiding this comment

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

Read through the PR, and the changes seem as described but I am not personally comfortable enough with the system and existing locking picture to approve myself.

The couple of questions that I do have are minor

ledger/txtail.go Show resolved Hide resolved
ledger/txtail.go Outdated Show resolved Hide resolved
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 confirmed that the AVM has its own check before calling BlockHdr() so the change to return headers from DB is fine from that perspective.

I don't want to weigh in on the locking discussion, since I don't know it well. But if you want my input, ping me and I'll spend more time understanding it.

I'm really glad to see this cleanup!

ledger/ledger.go Show resolved Hide resolved
data/ledger_test.go Outdated Show resolved Hide resolved
ledger/txtail.go Outdated Show resolved Hide resolved
ledger/txtail.go Show resolved Hide resolved
ledger/ledger.go Show resolved Hide resolved
ledger/ledger.go Show resolved Hide resolved
@algorandskiy
Copy link
Contributor

@algonautshant there are some linter and benchmark failures like

Raw Output:
  data/transactions/logic/assembler.go:1: : # github.com/algorand/go-algorand/data/transactions/logic [github.com/algorand/go-algorand/data/transactions/logic.test]
  Error: [Lint Errors] reported by reviewdog 🐶
  cannot use ep.Ledger (variable of type LedgerForLogic) as LedgerForSignature value in assignment: LedgerForLogic does not implement LedgerForSignature (missing method BlockHdr) (typecheck)

Please take a look, most likely some benchmark mock is missing BlockHdr method

@algorandskiy algorandskiy requested a review from cce August 1, 2023 21:00
algorandskiy
algorandskiy previously approved these changes Aug 1, 2023
algorandskiy
algorandskiy previously approved these changes Aug 2, 2023
@algorandskiy algorandskiy merged commit 8c87fa5 into algorand:master Aug 2, 2023
9 checks passed
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