std::shared_ptr based CTransaction storage in mempool

sipa commented at 4:04 pm on May 30, 2016: member

As suggested in #8068 (comment), using shared_ptr’s for mempool transactions has many advantages. This pull request introduces the basic changes, and removes many CTransaction copying operations from message processing as a result (only item 2 and 3 from the list of possible improvements from the linked comment).

gmaxwell commented at 4:15 pm on May 30, 2016: contributor

This should also simplify the compactblocks PR once it’s changed to make use of this. The reduction in memory usage for the relay pool would be nice– would make it reasonable to make the relay pool last a fair bit longer.

Concept ACK.

sipa force-pushed on May 30, 2016

paveljanik commented at 7:40 pm on May 30, 2016: contributor

This emits a lot of warnings:

0./memusage.h:76:11: warning: private field 'class_type' is not used [-Wunused-private-field]
1    void* class_type;
2          ^
3./memusage.h:77:9: warning: private field 'use_count' is not used [-Wunused-private-field]
4    int use_count;
5        ^
6./memusage.h:78:9: warning: private field 'weak_count' is not used [-Wunused-private-field]
7    int weak_count;
8        ^
93 warnings generated.

sipa force-pushed on May 30, 2016

sipa commented at 8:18 pm on May 30, 2016: member

@paveljanik Addressed, I think.

sipa force-pushed on May 30, 2016

in src/memusage.h: in 4042a03525 outdated

125+static inline size_t DynamicUsage(const std::shared_ptr<X>& p)
126+{
127+    // A shared_ptr can either use a single continuous memory block for both
128+    // the counter and the storage (when using std::make_shared), or separate.
129+    // We can't observe the difference, however, so assume the worst.
130+    return p ? MallocUsage(sizeof(X)) + MallocUsage(sizeof(stl_shared_counter<int>)) : 0;

dcousens commented at 5:00 am on May 31, 2016:

Is there any reference for these assumptions? stl_shared_counter looks “right”, but short on investigating the inner workings of my local STL, is this guaranteed?

paveljanik commented at 5:12 am on May 31, 2016:

@dcousens See e.g. http://en.cppreference.com/w/cpp/memory/shared_ptr, Implementation notes. I do not think we can be exact here, but this is good approximation.

dcousens commented at 5:14 am on May 31, 2016:

But why <int> over <size_t>? Wouldn’t size_t be the worst case for what could definitely be considered a “typical” implementation?

dcousens commented at 5:16 am on May 31, 2016:

In a typical implementation, std::shared_ptr holds only two pointers:

the stored pointer (one returned by get());

a pointer to control block.

Couldn’t we find a way to use sizeof(control_block_t)

paveljanik commented at 5:17 am on May 31, 2016:

The reference says that use_count is long (http://en.cppreference.com/w/cpp/memory/shared_ptr/use_count).

dcousens commented at 5:00 am on May 31, 2016: contributor

utACK 4042a03

in src/txmempool.cpp: in 4042a03525 outdated

22@@ -23,18 +23,18 @@ CTxMemPoolEntry::CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee,
23                                  int64_t _nTime, double _entryPriority, unsigned int _entryHeight,
24                                  bool poolHasNoInputsOf, CAmount _inChainInputValue,
25                                  bool _spendsCoinbase, unsigned int _sigOps, LockPoints lp):
26-    tx(_tx), nFee(_nFee), nTime(_nTime), entryPriority(_entryPriority), entryHeight(_entryHeight),

paveljanik commented at 5:13 am on May 31, 2016:

Just a side note: the original code is using _x here to prevent shadowing… (#8109)

sipa commented at 5:16 am on May 31, 2016: member

@dcousens My STL implementation uses _Atomic_word, which is a typedef’d int. Using size_t would be incorrect.

dcousens commented at 5:18 am on May 31, 2016: contributor

@dcousens My STL implementation uses _Atomic_word, which is a typedef’d int. Using size_t would be incorrect.

Sure, I made that assumption based on the fact you did use an int, but, what about others? Is there no relevant/accessible typedef that we could use here?

sipa commented at 5:22 am on May 31, 2016: member

It’d be helpful if someone can investigate libc++ and msvc++.

sipa commented at 7:54 am on May 31, 2016: member

@paveljanik But that’s the return value of use_count; not its internal range

sipa commented at 11:21 am on May 31, 2016: member

@dcousens If we find out what the data types are for the control blocks in different stl implementations, I guess we can just directly sizeof them. These are not things that easily change from one version to another.

sipa force-pushed on May 31, 2016

sipa commented at 2:18 pm on May 31, 2016: member

Simplified the resulting code a bit by introducing a TxMempoolInfo struct (contain a tx shared pointer, feerate, and entry time), and using that within the inv/mempool processing (rebase on top of #8080 needed access to entry time).

sipa force-pushed on May 31, 2016

sipa commented at 5:55 pm on May 31, 2016: member

Rebased on top of #8082, and modified the relay pool to use shared_ptr’s as well (so it doesn’t duplicate mempool storage anymore).

sipa commented at 6:07 pm on May 31, 2016: member

@dcousens It seems libc++ uses long for the shared_ptr counters, so I’ve changed the struct to convervatively use size_t,

sipa force-pushed on May 31, 2016

sipa force-pushed on Jun 1, 2016

dcousens commented at 2:49 am on June 2, 2016: contributor

@sipa :+1: I couldn’t find any relevant typedef that was consistent

dcousens commented at 2:51 am on June 2, 2016: contributor

reACK 74a1bb2

in src/memusage.h: in 74a1bb24ac outdated

69@@ -70,6 +70,15 @@ struct stl_tree_node
70     X x;
71 };
72 
73+struct stl_shared_counter
74+{
75+    /* Various platforms use different sized counters here.
76+     * Convervatively assume that they won't be larger than size_t. */

paveljanik commented at 11:24 am on June 2, 2016:

Conservatively

sipa commented at 10:32 pm on June 4, 2016:

Fixed.

gmaxwell commented at 5:50 pm on June 4, 2016: contributor

tested ACK.

gmaxwell commented at 7:09 pm on June 4, 2016: contributor

needs rebase (for #7997, I believe)

sipa force-pushed on Jun 4, 2016

Add support for unique_ptr and shared_ptr to memusage 1b9e6d3c1a

Switch CTransaction storage in mempool to std::shared_ptr 8d39d7a2cf

sipa force-pushed on Jun 4, 2016

sipa commented at 10:32 pm on June 4, 2016: member

Trivially rebased.

Optimize the relay map to use shared_ptr's

* Switch mapRelay to use shared_ptr<CTransaction>
* Switch the relay code to copy mempool shared_ptr's, rather than copying
  the transaction itself.
* Change vRelayExpiration to store mapRelay iterators rather than hashes
  (smaller and faster).

dbfb426b96

Optimization: don't check the mempool at all if no mempool req ever e9b4780b29

Optimization: use usec in expiration and reuse nNow c2a4724642

in src/main.cpp: in 89ffe945ba outdated

4517                 {
4518-                    LOCK(cs_mapRelay);
4519-                    map<uint256, CTransaction>::iterator mi = mapRelay.find(inv.hash);
4520+                    auto mi = mapRelay.find(inv.hash);
4521                     if (mi != mapRelay.end()) {
4522                         tx = (*mi).second;

kazcw commented at 11:29 pm on June 5, 2016:

Removing cs_mapRelay makes the bool push mechanism unnecessary since the conditionals can now be chained, allowing a simpler direct PushMessage that would also save a shared_ptr copy in this case.

sipa commented at 0:10 am on June 6, 2016:

Nice catch, fixed.

sipa force-pushed on Jun 6, 2016

laanwj commented at 9:10 am on June 7, 2016: member

If you get an error about missing symbols during linking while testing this, you need to clean your git tree (or preferably, just rm -rf your build directory if you’re doing out-of-tree builds).

Get rid of CTxMempool::lookup() entirely 288d85ddf2

in src/txmempool.cpp: in c2a4724642 outdated

842+    if (i == mapTx.end())
843+        return nullptr;
844+    return i->GetSharedTx();
845 }
846 
847 bool CTxMemPool::lookup(uint256 hash, CTransaction& result) const

laanwj commented at 11:07 am on June 7, 2016:

Is there value to having both get() and lookup()? (they seem to do the same just a slightly different API, this could be confusing)

sipa commented at 11:19 am on June 7, 2016:

Let’s see if I can get rid of that.

sipa commented at 11:45 am on June 7, 2016:

Fixed.

laanwj commented at 11:49 am on June 7, 2016: member

code review ACK https://github.com/bitcoin/bitcoin/pull/8126/commits/288d85ddf2e0a0c9d25a23db56052883170466d0, also testing on one node via https://github.com/sipa/bitcoin/tree/compactblocks which is rebased on this

laanwj merged this on Jun 8, 2016

laanwj closed this on Jun 8, 2016

laanwj referenced this in commit a7c41f2de0 on Jun 8, 2016

codablock referenced this in commit a224a594a8 on Sep 16, 2017

codablock referenced this in commit 2c4dd073f4 on Sep 19, 2017

codablock referenced this in commit ca699cebaa on Dec 22, 2017

andvgal referenced this in commit aaab493bec on Jan 6, 2019

furszy referenced this in commit 7b2b9d048e on Sep 24, 2020

furszy referenced this in commit 1f010c0969 on Jan 23, 2021

zkbot referenced this in commit 1d7ed06174 on Aug 13, 2021

zkbot referenced this in commit 56b5f95897 on Aug 17, 2021

MarcoFalke locked this on Sep 8, 2021

std::shared_ptr based CTransaction storage in mempool #8126