Avoid mapTx lookup in CTxMemPool::UpdateTransactionsFromBlock #13193

pull promag wants to merge 1 commits into bitcoin:master from promag:2018-05-avoid-maptx-lookup changing 1 files +7 −6

promag commented at 11:12 PM on May 8, 2018: member

This trivial patch prevents an expensive operation (mapTx.find()) by early testing an existing and cheaper condition.
fanquake added the label Mempool on May 8, 2018
in src/txmempool.cpp:141 in 3cc141d4ad outdated
```
 135 | @@ -136,14 +136,15 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<uint256> &vHashes
 136 |          // include them, and update their setMemPoolParents to include this tx.
 137 |          for (; iter != mapNextTx.end() && iter->first->hash == hash; ++iter) {
 138 |              const uint256 &childHash = iter->second->GetHash();
 139 | +            // We can skip updating entries that are in the block (which are
 140 | +            // already accounted for).
 141 | +            if (setAlreadyIncluded.count(childHash)) continue;
```
andreip commented at 5:44 AM on May 9, 2018:

one effect of adding this here would be that setChildren.insert won't be called for those that are skipped here anymore, but since setChildren is locally declared and only used once below, it looks fine.

This change lgtm since it's only adding a check to be called earlier to avoid an unnecessary mapTx.find() call and the rest of the boolean looks the same.

promag commented at 6:22 AM on May 9, 2018:

Yes, I can insert in setChildren and still avoid the lookup though, but like you said, it's not needed.
murrayn commented at 9:42 AM on May 15, 2018: contributor

@promag Is setAlreadyIncluded.count(childHash) the most common case? And, can its result be affected by the call to setChildren.insert(childIter).second?
promag commented at 1:08 PM on May 15, 2018: member

@promag Is setAlreadyIncluded.count(childHash) the most common case? And, can its result be affected by the call to setChildren.insert(childIter).second? @murrayn the idea is to avoid the childIter = mapTx.find call, which setChildren.insert(childIter) depends on.
promag commented at 11:46 AM on May 23, 2018: member

ping @morcos.
MarcoFalke commented at 1:11 PM on May 23, 2018: member

@promag It seems the first comment does not include any motivation and background.
promag renamed this:
~~Avoid 2nd mapTx lookup in CTxMemPool::UpdateTransactionsFromBlock~~
Avoid mapTx lookup in CTxMemPool::UpdateTransactionsFromBlock
on May 23, 2018
mempool: Avoid mapTx lookup in CTxMemPool::UpdateTransactionsFromBlock 4ed1871640
promag force-pushed on May 23, 2018
promag commented at 9:07 PM on May 23, 2018: member

@MarcoFalke improved first comment and reworded commit and PR title (removed 2nd).
MarcoFalke added the label Refactoring on May 23, 2018
DrahtBot commented at 11:49 PM on July 22, 2018: member

The last travis run for this pull request was 60 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
DrahtBot closed this on Jul 22, 2018
DrahtBot reopened this on Jul 22, 2018
promag commented at 4:02 PM on November 26, 2018: member

This change only saves some mapTx lookups, which in turn isn't that expensive considering the current size.
promag closed this on Nov 26, 2018
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels