Page MenuHomePhabricator

Remove mapLinks in favor of entry inlined structs with iterator type erasure
ClosedPublic

Authored by PiRK on Sep 23 2021, 15:03.

Details

Summary

PR description:

Remove CTxMempool::mapLinks data structure member

Currently we have a peculiar data structure in the mempool called maplinks. Maplinks job is to track the in-pool children and parents of each transaction. This PR can be primarily understood and reviewed as a simple refactoring to remove this extra data structure, although it comes with a nice memory and performance improvement for free.

Maplinks is particularly peculiar because removing it is not as simple as just moving it's inner structure to the owning CTxMempoolEntry. Because TxLinks (the class storing the setEntries for parents and children) store txiters to each entry in the mempool corresponding to the parent or child, it means that the TxLinks type is "aware" of the boost multiindex (mapTx) it's coming from, which is in turn, aware of the entry type stored in mapTx. Thus we used maplinks to store this entry associated data we in an entirely separate data structure just to avoid a circular type reference caused by storing a txiter inside a CTxMempoolEntry.

It turns out, we can kill this circular reference by making use of iterator_to multiindex function and std::reference_wrapper. This allows us to get rid of the maplinks data structure and move the ownership of the parents/child sets to the entries themselves.

The benefit of this good all around, for any of the reasons given below the change would be acceptable, and it doesn't make the code harder to reason about or worse in any respect (as far as I can tell, there's no tradeoff).

Simpler ownership model

No longer having to consistency check that mapLinks did have records for our CTxMempoolEntry, impossible to have a mapLinks entry outlive or incorrectly die before a CTxMempoolEntry.

Memory Usage

We get rid of a O(Transactions) sized map in the mempool, which is a long lived data structure.

Performance

If you have a CTxMemPoolEntry, you immediately know the address of it's children/parents, rather than having to do a O(log(Transactions)) lookup via maplinks (which we do very often). We do it in so many places that a true benchmark has to look at a full running node, but it is easy enough to show an improvement in this case.

The ComplexMemPool shows a good coherence check that we see the expected result of it being 12.5% faster / 1.14x faster.

The ComplexMemPool benchmark only checks doing addUnchecked and TrimToSize for 800 transactions. While this bench does a good job of hammering the relevant types of function, it doesn't test everything.

Subbing in 5000 transactions shows a that the advantage isn't completely wiped out by other asymptotic factors (this isn't the only bottleneck in growing the mempool), but it's only a bit proportionally slower (10.8%, 1.12x), which adds evidence that this will be a good change for performance minded users.

This is a backport of core#19478

Test Plan

ninja all check-all

Performance improvement confirmed with bitcoin-bench:

$ diff before.log after.log  | grep ComplexMemPool
  |             ns/elem |              elem/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:---------- 
< |      199,336,736.00 |                5.02 |    0.4% |      2.21 | `ComplexMemPool`
> |      181,013,454.00 |                5.52 |    0.4% |      1.99 | `ComplexMemPool`

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Sep 23 2021, 15:03
PiRK edited the test plan for this revision. (Show Details)
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/txmempool.cpp
769 ↗(On Diff #30150)

GetId()

This revision is now accepted and ready to land.Sep 27 2021, 08:56
PiRK edited the test plan for this revision. (Show Details)

address review and rebase onto master

This revision was landed with ongoing or failed builds.Sep 27 2021, 09:17
This revision was automatically updated to reflect the committed changes.