Improve transaction inclusion when block is nearly full #111
Conversation
jstarry
force-pushed
the
fix/improve-block-inclusion
branch
from
3d2d30a to
5891408
Compare
| @@ -470,33 +470,40 @@ impl Consumer { | |||
| chunk_offset: usize, | |||
| pre_results: impl Iterator<Item = Result<(), TransactionError>>, | |||
| ) -> ProcessTransactionBatchOutput { | |||
| // Lock accounts so that other threads cannot encode transactions that | |||
There was a problem hiding this comment.
Follow-up(?): seems these 3 operations are essentially creating a batch (lock, cost model, unlock).
Do you think this makes sense to create a function for?
| }) | ||
| )); | ||
| // Unlock accounts for any transactions that were not selected for the block; | ||
| batch.unlock_failures( |
There was a problem hiding this comment.
I doubt it outweighs the benefits of this, but did you measure the impact of this change on performance when we have many small batches? Maybe run bench-tps as a sanity check on this, using --num-conflict-groups 1
We're grabbing the account locks lock 3 times instead of twice per batch now, so could lead to more contention for that lock.
There was a problem hiding this comment.
banking-bench will be better probably to highlight any differences.
There was a problem hiding this comment.
I ran the banking-bench with write lock contention and this change brings in a pretty consistent perf hit that I can't really debug. After discussing with @sakridge I think it's best to just skip this change to avoid the backport risk to v1.17 and wait until v1.18 and the new scheduler..
cargo run --release --manifest-path banking-bench/Cargo.toml -- --write-lock-contention full --iterations 100
before
[total_sent: 115200, base_tx_count: 36864, txs_processed: 140306, txs_landed: 103442, total_us: 6721644, tx_total_us: 6620406]
{'name': 'banking_bench_total', 'median': '17138.66'}
{'name': 'banking_bench_tx_total', 'median': '17400.75'}
{'name': 'banking_bench_success_tx_total', 'median': '15389.39'}
after
[total_sent: 115200, base_tx_count: 36864, txs_processed: 142769, txs_landed: 105905, total_us: 7230649, tx_total_us: 7135781]
{'name': 'banking_bench_total', 'median': '15932.18'}
{'name': 'banking_bench_tx_total', 'median': '16143.99'}
{'name': 'banking_bench_success_tx_total', 'median': '14646.68'}
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #111 +/- ##
=========================================
- Coverage 81.8% 81.8% -0.1%
=========================================
Files 838 838
Lines 225913 226002 +89
=========================================
+ Hits 184935 184967 +32
- Misses 40978 41035 +57 |
Problem
When a leader prepares a transaction batch for execution, it needs to check if the accounts for each transaction can be locked and it needs to check if the transaction will fit within the cost tracker limits. Currently the banking stage consumer first selects transactions that can fit within remaining block space and THEN locks the accounts. If any account locking fails, the consumer is needlessly holding onto reserved block space until the consumer finishes processing that batch of transactions.
Summary of Changes
process_and_record_transactions_with_pre_resultsto first lock as many transactions as possible in a batch, then reserve block space, and then finally unlock any transactions that didn't fit in the block.Fixes solana-labs#34825