Improve performance when promoting transaction from next layers #5920
Conversation
|
fab-10
force-pushed
the
txpool-promotion-performance
branch
from
e289dbd to
f97d749
Compare
Signed-off-by: Fabio Di Fabio <[email protected]>
fab-10
force-pushed
the
txpool-promotion-performance
branch
from
f97d749 to
17220d8
Compare
| final Predicate<PendingTransaction> promotionFilter, | ||
| final long freeSpace, | ||
| final int freeSlots) { | ||
| long accSpace = 0; |
There was a problem hiding this comment.
here acc stays for accumulated space, to keep how much space has been accumulated by the selected txs till now, do you think it is better to rename it?
There was a problem hiding this comment.
renaming to accumulatedSpace
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/transactions/layered/EndLayer.java
Outdated
Show resolved
Hide resolved
.../org/hyperledger/besu/ethereum/eth/transactions/layered/AbstractPrioritizedTransactions.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Sally MacFarlane <[email protected]> Signed-off-by: Fabio Di Fabio <[email protected]>
| for (final var candidateTx : senderTxs.values()) { | ||
| if (promotionFilter.test(candidateTx)) { | ||
| accSpace += candidateTx.memorySize(); | ||
| if (promotedTxs.size() < freeSlots && accSpace <= freeSpace) { |
There was a problem hiding this comment.
maybe a comment to explain why < for slots and <= for space
There was a problem hiding this comment.
can I ask what you get from the code about the difference? to understand what needs clarification
There was a problem hiding this comment.
freeSlots and freeSpace have similar names - how much is free - so why does the if statement check < for one and <= for the other?
why not <= for freeSlots?
There was a problem hiding this comment.
slots always increment by 1, while for space it could be any amount, so you can read it like that
if (promotedTxs.size() + 1 <= freeSlots && accumulatedSpace <= freeSpace)
but the +1 is redundant knowing that promotedTxs.size() can only increment by 1 at time, and we can shortcut with <
There was a problem hiding this comment.
right so it's because the accumulatedSpace is already incremented at this point but for the slots it happens after. makes sense. it's ok for me :)
| final Predicate<PendingTransaction> promotionFilter, | ||
| final long freeSpace, | ||
| final int freeSlots) { | ||
| long accSpace = 0; |
…rledger#5920) Signed-off-by: Fabio Di Fabio <[email protected]> Co-authored-by: Sally MacFarlane <[email protected]> Signed-off-by: Justin Florentine <[email protected]>
…rledger#5920) Signed-off-by: Fabio Di Fabio <[email protected]> Co-authored-by: Sally MacFarlane <[email protected]>
PR description
During the implementation of #5921, a performance issue surfaced in the way the promotion of transactions, from lower layers, was implemented.
The issue was that, to keep the code simple, the promotion was done for one tx at once, but with the improvement on the prioritized layer done #5921, this approach is no more practical since result in a quadratic complexity (number of confirmed txs per number of senders in the ready layer), so the solution is to do the promotion only once after all the confirmed txs have been processed, so the time is linear.