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

api: optimize /transactions/pending/{txid} endpoint #5891

Merged
merged 6 commits into from Jan 17, 2024

Conversation

algorandskiy
Copy link
Contributor

@algorandskiy algorandskiy commented Jan 4, 2024

Summary

  • Use txtail data to find txid in memory (only txns with LastValid > current round) in addition to iterating blocks
  • RPS: before: 10, after: 450
  • Another optimization possible with caching blocks that would bump RPS to 30k
  • It is possible to completely get rid of blocks iteration by one of the following:
    • Extend txtail to have MaxTxnLife old rounds. Requires lots of memory.
    • Make a new cache in AlgorandFullNode for txids: 26k txns/block * 1000 rounds => need 25 bits for all combos => 4 bytes (32bits) of txid instead of 32 bytes of a full txid would require 26M*4b = 100MB of RAM.
  • Estimated extra memory footprint is 52MB = 26k txns/block * 1000 rounds * 2 bytes per round delta.

Test Plan

Added unit test for new txtail logic

* Use txtail data to find txid in memory instead of iterating blocks
* RPS: before: 10, after: 450
* Another optimization possible with caching blocks that would
  bump RPS to 30k
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 like how simple this change is!

ledger/txtail.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 4, 2024

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (3a80a40) 55.92% compared to head (002cebf) 55.89%.
Report is 15 commits behind head on master.

Files Patch % Lines
node/node.go 0.00% 11 Missing ⚠️
ledger/txtail.go 88.46% 2 Missing and 1 partial ⚠️
ledger/ledger.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5891      +/-   ##
==========================================
- Coverage   55.92%   55.89%   -0.04%     
==========================================
  Files         477      477              
  Lines       67432    67458      +26     
==========================================
- Hits        37713    37703      -10     
- Misses      27164    27195      +31     
- Partials     2555     2560       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@algorandskiy
Copy link
Contributor Author

While unit testing I found t.lastValid is aggressively trimmed and does not have all txids for MaxTxnLife rounds back, it only has txids for txns with LastValid in future from current.

@algorandskiy algorandskiy reopened this Jan 5, 2024
@algorandskiy
Copy link
Contributor Author

algorandskiy commented Jan 5, 2024

After offline discussion decided to still have it and use txTail lookup as a filter that probably vast majority of use cases when people immediately lookup for recently submitted transactions with LastValid at least few rounds in future.

@algorandskiy algorandskiy marked this pull request as ready for review January 5, 2024 16:02
@algorandskiy algorandskiy changed the title WIP: api: optimize /transactions/pending/{txid} endpoint api: optimize /transactions/pending/{txid} endpoint Jan 5, 2024
ledger/ledger.go Outdated Show resolved Hide resolved
@algorandskiy
Copy link
Contributor Author

Relevant ticket #3967

ledger/txtail.go Outdated Show resolved Hide resolved
ledger/txtail.go Outdated Show resolved Hide resolved
ledger/txtail.go Outdated Show resolved Hide resolved
node/node.go Show resolved Hide resolved
gmalouf
gmalouf previously approved these changes Jan 10, 2024
Copy link
Contributor

@gmalouf gmalouf left a comment

Choose a reason for hiding this comment

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

Added a question or two, but overall this makes sense to me. I second JJ's suggestion around uint vs int, but will not block approval on it.

@gmalouf gmalouf merged commit 5e5c16a into algorand:master Jan 17, 2024
18 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.

None yet

3 participants