perf(tx-pool): reuse write lock to insert txs batch #12806
Conversation
hai-rise
force-pushed
the
reuse-write-lock
branch
from
c3bdc95 to
feab580
Compare
|
for the rpc part, this could be a dedicated task that receives all new txs, batches them and submits them in batches |
hai-rise
force-pushed
the
reuse-write-lock
branch
from
feab580 to
cd971ae
Compare
| sidecar: sidecar.clone(), | ||
| }, | ||
| propagate: true, | ||
| test_pool.add_transactions( |
There was a problem hiding this comment.
I changed the test to use the pub function to ensure the pool always enforces the size in "real usage". The previous tested two private functions used in conjunction which wasn't as guaranteed (removing discarding in add_transactions would still pass the test).
I cannot work on this immediately so created #12811. Maybe someone familiar with the RPC codebase can kill it better! |
|
@mattsse Kind reminder that this is available for review again 🙏. |
| if discarded.is_empty() { | ||
| return added | ||
| } | ||
| if !discarded.is_empty() { |
There was a problem hiding this comment.
This is only refactoring that doesn't change semantics.
Previously:
- If
discardedis empty then early returnadded - Else make some changes to
added - Return
addedat the end
Now:
- If
discardedis not empty then make changes toadded - Return
addedat the end
It also groups discard handling in a self-explanatory if scope 🙏.
| // delete any blobs associated with discarded blob transactions | ||
| self.delete_discarded_blobs(discarded.iter()); |
There was a problem hiding this comment.
this logic has been removed now, right?
There was a problem hiding this comment.
It's moved to the only call site (add_transactions) now, in the if !discarded.is_empty() check.
|
if you want me to take care of conflicts, please open prs from non-org forks :) pending conflicts |
hai-rise
force-pushed
the
reuse-write-lock
branch
from
cd971ae to
7f8072a
Compare
|
@mattsse No worries, let us fix all the conflicts (done for this PR) 🙏. |
Currently, each transaction insertion competes for a write lock of the pool. This can be very congested for performance chains that expect 100k+ new transactions every second. Even more challenging if the block time is small -- canonical state changes also compete for the write lock several times a second.
Ideally, the RPC server can buffer transactions to insert in a batch when the pool write lock is unavailable, instead of competing for the write lock (and block anyway) for every insertion request.
I guess this PR is a baby step towards that 😅, to start reusing a single write lock in
PoolInner::add_transactions.