This more closely approximates the desirability of a given transaction for mining, and should result in less re-sorting when transactions get removed from the mempool after being mined.
I measured this as approximately a 5% speedup in removeForBlock.
298+
299+ // Calculate which score to use for an entry (avoiding division).
300+ // Picks min(feerate, feerate_with_ancestors)
301+ bool UseAncestorScore(const CTxMemPoolEntry &a) const
302+ {
303+ double f1 = (double)a.GetModifiedFee() * a.GetSizeWithAncestors();
273- double aFees = a.GetModFeesWithAncestors();
274- double aSize = a.GetSizeWithAncestors();
275+ bool fUseAAncestors = UseAncestorScore(a);
276+ bool fUseBAncestors = UseAncestorScore(b);
277+
278+ double aModFee = fUseAAncestors ? a.GetModFeesWithAncestors() : a.GetModifiedFee();
278+ double aModFee = fUseAAncestors ? a.GetModFeesWithAncestors() : a.GetModifiedFee();
279+ double aSize = fUseAAncestors ? a.GetSizeWithAncestors() : a.GetTxSize();
280
281- double bFees = b.GetModFeesWithAncestors();
282- double bSize = b.GetSizeWithAncestors();
283+ double bModFee = fUseBAncestors ? b.GetModFeesWithAncestors() : b.GetModifiedFee();
Instead of factoring out UseAncestorScore, do (missing comments):
0void GetModFeeAndSize(const CTxMemPoolEntry& a, int64_t& mod_fee, int64_t& size) {
1 int64_t f1 = a.GetModifiedFee() * a.GetSizeWithAncestors();
2 int64_t f2 = a.GetModFeesWithAncestors() * a.GetTxSize();
3 if (f1 > f2) {
4 mod_fee = a.GetModFeesWithAncestors();
5 size = a.GetSizeWithAncestors();
6 } else {
7 mod_fee = a.GetModifiedFee();
8 size = a.GetTxSize();
9 }
10}
295
296 return f1 > f2;
297 }
298+
299+ // Calculate which score to use for an entry (avoiding division).
300+ // Picks min(feerate, feerate_with_ancestors)
..
@promag Thanks for taking a look. I mimicked CompareTxMempoolEntryByDescendantScore in this PR to make review easier, since the two concepts have a symmetry.
The use of double for the intermediate value is to prevent overflow of an int64_t, which is theoretically possible. That’s also why there’s an explicit double cast before multiplication.
I’ll rewrite the helper function along the lines of your suggestion.
This more closely approximates the desirability of a given transaction for
mining.
The use of double for the intermediate value is to prevent overflow of an int64_t, which is theoretically possible.
How about http://releases.llvm.org/4.0.0/tools/clang/docs/LanguageExtensions.html#checked-arithmetic-builtins?
@promag The style of where to put the & doesn’t seem to be in our style guide and is inconsistent already in this file, so I’m going to leave it the way I have it (which happens to be my preference for where to put the & :) ).
I don’t think it’s a problem for code-correctness if the arithmetic overflows, so I’d rather just handle it as we currently do than introduce complication from calculating things two ways.
utACK 9a51319.
I’m going to leave it the way I have it
Fine.
so I’d rather just handle it as we currently do
It was just curiosity. Maybe GetModFeeAndSize values could be cached in CTxMemPoolEntry to improve even further? (adding 128 bytes per entry)
utACK 9a51319578091234fdd218a1eb144d517ea82b85
I measured this as approximately a 5% speedup in removeForBlock.
I’m always interested to know how you benchmark these things, in case you want to post details.
The style of where to put the & doesn’t seem to be in our style guide
Not important, but we do currently use the opposite setting for the code formatter:
I measured this as approximately a 5% speedup in removeForBlock.
I’m always interested to know how you benchmark these things, in case you want to post details. @ryanofsky I ran through nearly 3 months of historical block & transaction data (roughly March-May 2017), which I record and can play back to a patched bitcoind. I ran two historical studies, one with this patch and one without, each with added benchmarks to specifically measure the time spent in removeForBlock.
MempoolAncestorIndexingTest with a case of a higher fee parent with a lower fee child (for which the sort order has changed).
Transaction selection for mining tracks ancestor feerates that are
modified based on transactions that have already been selected. This
commit de-duplicates the code so that the ancestor feerate sorting used
by the mempool can also be directly applied to the miner.
351@@ -352,7 +352,7 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda
352 // Try to compare the mapTx entry to the mapModifiedTx entry
353 iter = mempool.mapTx.project<0>(mi);
354 if (modit != mapModifiedTx.get<ancestor_score>().end() &&
355- CompareModifiedEntry()(*modit, CTxMemPoolModifiedEntry(iter))) {
356+ CompareTxMemPoolEntryByAncestorFee()(*modit, CTxMemPoolModifiedEntry(iter))) {
Yes – this change was only because I wanted to delete the code for CompareModifiedEntry, which is otherwise no longer needed.
all modifiedTx entries also have their parents in modifiedTx
Not quite – the parents of entries in mapModifiedTx are not in mapModifiedTx if they have been included in the candidate block. Also a parent could be missing from mapModifiedTx if we already considered it for inclusion and it failed for some reason (eg the package it’s part of would be too big).
