Skip to content

feat(sequencer): various updates to mempool PR#1349

Merged
lobstergrindset merged 2 commits intolilyjjo/sequencer_app_mempool_pendingfrom
fraser/sequencer_app_mempool_pending
Aug 13, 2024
Merged

feat(sequencer): various updates to mempool PR#1349
lobstergrindset merged 2 commits intolilyjjo/sequencer_app_mempool_pendingfrom
fraser/sequencer_app_mempool_pending

Conversation

@Fraser999
Copy link
Contributor

Summary

This is effectively an update to the existing PR #1323.

Background

While reviewing #1323, it seemed more expedient to open a PR against it rather than adding many GH comments (this was discussed with @Lilyjjo).

As always, there are more changes here than I initially expected. The main thrust of the changes was to try and use the type system to avoid certain classes of errors, and to make things a little more canonical where possible.

Changes

  • At a high level, there are now separate types dealing with pending txs and parked txs: PendingTransactionsForAccount, PendingTransactions, ParkedTransactionsForAccount and ParkedTransactions. For the ones dealing with a single account, much of their logic is common, so a trait TransactionsForAccount was introduced to express that shared logic. For the ones dealing with collection of accounts' txs, a struct was introduced: TransactionsContainer generic over the type of the account-specific container T: TransactionsForAccount. This allowed for the shared logic to be expressed on the generic impl, with methods which are only relevant to pending being added to only that specialization, and similarly for the parked impl.
  • Extraneous fields were removed to avoid potential synchronization issues (e.g. Mempool::all and TransactionContainer::all)
  • Reduced scope of several of the locks made on the RwLocks
  • Various other smaller changes

Testing

  • No additional tests were added, the existing tests were modified.
  • Benchmarks were run and reveal very promising results. These are a reflection of the changes made in the original PR rather than this one. Example output is included below, and for reference, the previous figures are available in the description of chore(sequencer): add mempool benchmarks #1238. Summary is that for a mempool with 100 txs, we're seeing roughly a two times speed increase, and at 100,000 txs, we're seeing roughly a ten times increase.
Example of running cargo bench -qp astria-sequencer on my Ryzen 7900X
Timer precision: 10 ns
astria_sequencer                     fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ mempool                                         │               │               │               │         │
   ╰─ benchmarks                                   │               │               │               │         │
      ├─ check_removed_comet_bft     41.1 µs       │ 47.7 µs       │ 41.96 µs      │ 42.81 µs      │ 100     │ 100
      ├─ builder_queue                             │               │               │               │         │
      │  ├─ mempool_with_100000_txs  9.578 ms      │ 13.59 ms      │ 10.09 ms      │ 10.17 ms      │ 100     │ 100
      │  ├─ mempool_with_10000_txs   613 µs        │ 821.4 µs      │ 734.7 µs      │ 723.7 µs      │ 100     │ 100
      │  ├─ mempool_with_1000_txs    55.37 µs      │ 68.09 µs      │ 59.3 µs       │ 59.44 µs      │ 100     │ 100
      │  ╰─ mempool_with_100_txs     9.377 µs      │ 14.2 µs       │ 9.913 µs      │ 10.21 µs      │ 100     │ 100
      ├─ insert                                    │               │               │               │         │
      │  ├─ mempool_with_100000_txs  2.79 ms       │ 3.327 ms      │ 2.944 ms      │ 2.968 ms      │ 100     │ 100
      │  ├─ mempool_with_10000_txs   137.9 µs      │ 277.3 µs      │ 201.4 µs      │ 199.2 µs      │ 100     │ 100
      │  ├─ mempool_with_1000_txs    15.38 µs      │ 24.64 µs      │ 18.73 µs      │ 18.78 µs      │ 100     │ 100
      │  ╰─ mempool_with_100_txs     7.303 µs      │ 11.24 µs      │ 7.579 µs      │ 7.686 µs      │ 100     │ 100
      ├─ pending_nonce                             │               │               │               │         │
      │  ├─ mempool_with_100000_txs  2.727 ms      │ 3.524 ms      │ 2.896 ms      │ 2.92 ms       │ 100     │ 100
      │  ├─ mempool_with_10000_txs   124.4 µs      │ 275.7 µs      │ 198.2 µs      │ 194.4 µs      │ 100     │ 100
      │  ├─ mempool_with_1000_txs    12.56 µs      │ 22.14 µs      │ 17.16 µs      │ 16.97 µs      │ 100     │ 100
      │  ╰─ mempool_with_100_txs     6.001 µs      │ 11.11 µs      │ 6.176 µs      │ 6.262 µs      │ 100     │ 100
      ├─ remove_tx_invalid                         │               │               │               │         │
      │  ├─ mempool_with_100000_txs  2.76 ms       │ 3.832 ms      │ 2.945 ms      │ 3.02 ms       │ 100     │ 100
      │  ├─ mempool_with_10000_txs   120.5 µs      │ 288.1 µs      │ 199.2 µs      │ 201 µs        │ 100     │ 100
      │  ├─ mempool_with_1000_txs    14.76 µs      │ 31.87 µs      │ 18.22 µs      │ 18.35 µs      │ 100     │ 100
      │  ╰─ mempool_with_100_txs     7.593 µs      │ 18.72 µs      │ 7.824 µs      │ 8.123 µs      │ 100     │ 100
      ╰─ run_maintenance                           │               │               │               │         │
         ├─ mempool_with_100000_txs  3.209 ms      │ 3.877 ms      │ 3.424 ms      │ 3.456 ms      │ 100     │ 100
         ├─ mempool_with_10000_txs   423.5 µs      │ 765.5 µs      │ 497.7 µs      │ 505.3 µs      │ 100     │ 100
         ├─ mempool_with_1000_txs    81 µs         │ 125.8 µs      │ 88.26 µs      │ 88.84 µs      │ 100     │ 100
         ╰─ mempool_with_100_txs     14.14 µs      │ 24.13 µs      │ 14.64 µs      │ 15.04 µs      │ 100     │ 100

@Fraser999 Fraser999 requested review from a team as code owners August 4, 2024 21:48
@Fraser999 Fraser999 requested a review from SuperFluffy August 4, 2024 21:48
@github-actions github-actions bot added the sequencer pertaining to the astria-sequencer crate label Aug 4, 2024
let mut all = self.all.write().await;
let mut pending = self.pending.write().await;
let mut parked = self.parked.write().await;
let (mut pending, mut parked) = join!(self.pending.write(), self.parked.write());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a possibility of deadlock here? I'm thinking if two threads calls this (or one of the other holding joins) at the same time, what happens if one thread gets the pending lock and the other the parked lock and then they just both squat on the locks? I tried reading into the join! macro more but it didn't seem to say what would happen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call - it could indeed deadlock! I'll revert these join! calls.

drop(parked);
for ttx in to_promote {
if let Err(error) = pending.add(ttx, current_account_nonce) {
error!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, where to these error logs go? Like do we track them anywhere?

Comment on lines 340 to 346
let tx1_replacement = mock_tx(1, &signing_key, "test");
assert_eq!(
mempool
.insert(tx1_replacement.clone(), 0)
.await
.unwrap_err(),
InsertionError::AlreadyPresent,
Copy link
Contributor

Choose a reason for hiding this comment

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

To preserve the original assert intent:

Suggested change
let tx1_replacement = mock_tx(1, &signing_key, "test");
assert_eq!(
mempool
.insert(tx1_replacement.clone(), 0)
.await
.unwrap_err(),
InsertionError::AlreadyPresent,
let tx1_replacement = mock_tx(1, &signing_key, "test_0");
assert_eq!(
mempool
.insert(tx1_replacement.clone(), 0)
.await
.unwrap_err(),
InsertionError::NonceTaken,

@lobstergrindset
Copy link
Contributor

This LGTM! Going to merge in to make the discussed changes to the current nonce checks and promotion logic

@lobstergrindset lobstergrindset merged commit f88bb01 into lilyjjo/sequencer_app_mempool_pending Aug 13, 2024
@lobstergrindset lobstergrindset deleted the fraser/sequencer_app_mempool_pending branch August 13, 2024 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sequencer pertaining to the astria-sequencer crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants