-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Improve performance when promoting transaction from next layers #5920
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
Changes from 2 commits
17220d8
0b4bfa9
b4e74e9
0b72427
c3e1fd8
02b6b59
ca97d5e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,6 @@ | |
| import org.hyperledger.besu.ethereum.mainnet.feemarket.FeeMarket; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.Collection; | ||
| import java.util.Comparator; | ||
| import java.util.HashMap; | ||
| import java.util.HashSet; | ||
|
|
@@ -126,36 +125,86 @@ protected void internalReplaced(final PendingTransaction replacedTx) { | |
| @Override | ||
| protected void internalBlockAdded(final BlockHeader blockHeader, final FeeMarket feeMarket) {} | ||
|
|
||
| /** | ||
| * We only want to promote transactions that have gap == 0, so there will be no gap in the prev | ||
| * layers. A promoted transaction is removed from this layer, and the gap data is updated for its | ||
| * sender. | ||
| * | ||
| * @param promotionFilter the prev layer's promotion filter | ||
| * @param freeSpace max amount of memory promoted txs can occupy | ||
| * @param freeSlots max number of promoted txs | ||
| * @return a list of transactions promoted to the prev layer | ||
| */ | ||
| @Override | ||
| public PendingTransaction promote(final Predicate<PendingTransaction> promotionFilter) { | ||
| final PendingTransaction promotedTx = | ||
| orderByGap.get(0).stream() | ||
| .map(txsBySender::get) | ||
| .map(NavigableMap::values) | ||
| .flatMap(Collection::stream) | ||
| .filter(promotionFilter) | ||
| .findFirst() | ||
| .orElse(null); | ||
|
|
||
| if (promotedTx != null) { | ||
| final Address sender = promotedTx.getSender(); | ||
| final var senderTxs = txsBySender.get(sender); | ||
| senderTxs.pollFirstEntry(); | ||
| processRemove(senderTxs, promotedTx.getTransaction(), PROMOTED); | ||
| if (senderTxs.isEmpty()) { | ||
| txsBySender.remove(sender); | ||
| orderByGap.get(0).remove(sender); | ||
| gapBySender.remove(sender); | ||
| } else { | ||
| final long firstNonce = senderTxs.firstKey(); | ||
| final int newGap = (int) (firstNonce - (promotedTx.getNonce() + 1)); | ||
| if (newGap != 0) { | ||
| updateGap(sender, 0, newGap); | ||
| public List<PendingTransaction> promote( | ||
| final Predicate<PendingTransaction> promotionFilter, | ||
| final long freeSpace, | ||
| final int freeSlots) { | ||
| long accSpace = 0; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. usedSpace?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. allocatedSpace?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. accumulated also works
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. renaming to |
||
| final List<PendingTransaction> promotedTxs = new ArrayList<>(); | ||
|
|
||
| final var zeroGapSenders = orderByGap.get(0); | ||
|
|
||
| search: | ||
| for (final var sender : zeroGapSenders) { | ||
| final var senderSeqTxs = getSequentialSubset(txsBySender.get(sender)); | ||
|
|
||
| for (final var candidateTx : senderSeqTxs.values()) { | ||
|
|
||
| if (promotionFilter.test(candidateTx)) { | ||
| accSpace += candidateTx.memorySize(); | ||
| if (promotedTxs.size() < freeSlots && accSpace <= freeSpace) { | ||
| promotedTxs.add(candidateTx); | ||
| } else { | ||
| // no room for more txs the search is over exit the loops | ||
| break search; | ||
| } | ||
| } else { | ||
| // skip remaining txs for this sender | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return promotedTx; | ||
| // remove promoted txs from this layer | ||
| promotedTxs.forEach( | ||
| promotedTx -> { | ||
| final var sender = promotedTx.getSender(); | ||
| final var senderTxs = txsBySender.get(sender); | ||
| senderTxs.remove(promotedTx.getNonce()); | ||
| processRemove(senderTxs, promotedTx.getTransaction(), PROMOTED); | ||
| if (senderTxs.isEmpty()) { | ||
| txsBySender.remove(sender); | ||
| orderByGap.get(0).remove(sender); | ||
| gapBySender.remove(sender); | ||
| } else { | ||
| final long firstNonce = senderTxs.firstKey(); | ||
| final int newGap = (int) (firstNonce - (promotedTx.getNonce() + 1)); | ||
| if (newGap != 0) { | ||
| updateGap(sender, 0, newGap); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| if (!promotedTxs.isEmpty()) { | ||
| // since we removed some txs we can try to promote from next layer | ||
| promoteTransactions(); | ||
| } | ||
|
|
||
| return promotedTxs; | ||
| } | ||
|
|
||
| private NavigableMap<Long, PendingTransaction> getSequentialSubset( | ||
| final NavigableMap<Long, PendingTransaction> senderTxs) { | ||
| long lastSequentialNonce = senderTxs.firstKey(); | ||
| for (final long nonce : senderTxs.tailMap(lastSequentialNonce, false).keySet()) { | ||
| if (nonce == lastSequentialNonce + 1) { | ||
| ++lastSequentialNonce; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| return senderTxs.headMap(lastSequentialNonce, true); | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a comment to explain why < for slots and <= for space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can I ask what you get from the code about the difference? to understand what needs clarification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
+1is redundant knowing thatpromotedTxs.size()can only increment by 1 at time, and we can shortcut with<There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)