std::shared_ptr based CTransaction storage in mempool #8126

sipa · 2016-05-30T16:04:08Z

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 · 2016-05-30T16:15:28Z

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.

paveljanik · 2016-05-30T19:40:10Z

This emits a lot of warnings:

./memusage.h:76:11: warning: private field 'class_type' is not used [-Wunused-private-field]
    void* class_type;
          ^
./memusage.h:77:9: warning: private field 'use_count' is not used [-Wunused-private-field]
    int use_count;
        ^
./memusage.h:78:9: warning: private field 'weak_count' is not used [-Wunused-private-field]
    int weak_count;
        ^
3 warnings generated.

sipa · 2016-05-30T20:18:37Z

@paveljanik Addressed, I think.

dcousens · 2016-05-31T05:00:04Z

src/memusage.h

+    // A shared_ptr can either use a single continuous memory block for both
+    // the counter and the storage (when using std::make_shared), or separate.
+    // We can't observe the difference, however, so assume the worst.
+    return p ? MallocUsage(sizeof(X)) + MallocUsage(sizeof(stl_shared_counter<int>)) : 0;


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?

@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.

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

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)

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

dcousens · 2016-05-31T05:00:21Z

utACK 4042a03

paveljanik · 2016-05-31T05:13:37Z

src/txmempool.cpp

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


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

sipa · 2016-05-31T05:16:11Z

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

dcousens · 2016-05-31T05:18:07Z

@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 · 2016-05-31T05:22:26Z

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

sipa · 2016-05-31T07:54:50Z

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

sipa · 2016-05-31T11:21:43Z

@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 · 2016-05-31T14:18:45Z

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 · 2016-05-31T17:55:47Z

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 · 2016-05-31T18:07:38Z

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

dcousens · 2016-06-02T02:49:34Z

@sipa 👍 I couldn't find any relevant typedef that was consistent

dcousens · 2016-06-02T02:51:58Z

reACK 74a1bb2

paveljanik · 2016-06-02T11:24:09Z

src/memusage.h

+struct stl_shared_counter
+{
+    /* Various platforms use different sized counters here.
+     * Convervatively assume that they won't be larger than size_t. */


Conservatively

gmaxwell · 2016-06-04T17:50:17Z

tested ACK.

gmaxwell · 2016-06-04T19:09:00Z

needs rebase (for #7997, I believe)

sipa · 2016-06-04T22:32:13Z

Trivially rebased.

kazcw · 2016-06-05T23:29:16Z

src/main.cpp

                // Send stream from relay memory
                bool push = false;
                {
-                    LOCK(cs_mapRelay);
-                    map<uint256, CTransaction>::iterator mi = mapRelay.find(inv.hash);
+                    auto mi = mapRelay.find(inv.hash);
                    if (mi != mapRelay.end()) {
                        tx = (*mi).second;


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.

Nice catch, fixed.

* 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).

laanwj · 2016-06-07T09:10:35Z

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).

laanwj · 2016-06-07T11:07:53Z

src/txmempool.cpp

-    return true;
+    if (i == mapTx.end())
+        return nullptr;
+    return i->GetSharedTx();
 }

 bool CTxMemPool::lookup(uint256 hash, CTransaction& result) const


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

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

laanwj · 2016-06-07T11:49:18Z

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

288d85d Get rid of CTxMempool::lookup() entirely (Pieter Wuille) c2a4724 Optimization: use usec in expiration and reuse nNow (Pieter Wuille) e9b4780 Optimization: don't check the mempool at all if no mempool req ever (Pieter Wuille) dbfb426 Optimize the relay map to use shared_ptr's (Pieter Wuille) 8d39d7a Switch CTransaction storage in mempool to std::shared_ptr (Pieter Wuille) 1b9e6d3 Add support for unique_ptr and shared_ptr to memusage (Pieter Wuille)

…pool 288d85d Get rid of CTxMempool::lookup() entirely (Pieter Wuille) c2a4724 Optimization: use usec in expiration and reuse nNow (Pieter Wuille) e9b4780 Optimization: don't check the mempool at all if no mempool req ever (Pieter Wuille) dbfb426 Optimize the relay map to use shared_ptr's (Pieter Wuille) 8d39d7a Switch CTransaction storage in mempool to std::shared_ptr (Pieter Wuille) 1b9e6d3 Add support for unique_ptr and shared_ptr to memusage (Pieter Wuille)

911e31c Make CBlock::vtx a vector of shared_ptr<CTransaction> (furszy) 9d27b75 Add deserializing constructors to CTransaction and CMutableTransaction (Pieter Wuille) 5f90940 Add serialization for unique_ptr and shared_ptr (Pieter Wuille) Pull request description: Inspired on bitcoin#9125 (with many more adaptations), preparation for bitcoin#8580. Ant work 🐜 Establishing the bases for the work, we will need to continue migrating `CTransaction` to `std::shared_ptr<const CTransaction>` where is possible. Example bitcoin#8126 (can find more in #1726 list). TODO: * back port final `CTransactionRef` implementation commit. EDIT: This is now depending on #1816 . ACKs for top commit: random-zebra: ACK 911e31c Fuzzbawls: ACK 911e31c Tree-SHA512: 61d937aba7dce4ac0867496e7f81ae8351dcdd60b4e72c4f0ed58152a7e556bf455836c766bc97bbca331227e5deed92fa5ce609fa63bb9cb71600b430dc4405

9b9c616 Fix missing zapwallettxes mode in wallet_hd.py functional test (furszy) d6d0ad9 [logs] fix zapwallettxes startup logs (John Newbery) 006c503 [wallet] fix zapwallettxes interaction with persistent mempool (John Newbery) c6d45c6 Adapting and connecting mempool_persist.py functional test to the test runner. (furszy) 4f26a4e Control mempool persistence using a command line parameter. (John Newbery) 5d949de [Qt] Do proper shutdown (Jonas Schnelli) e60da98 Allow shutdown during LoadMempool, dump only when necessary (Jonas Schnelli) c0a0e81 Moving TxMempoolInfo tx member to CTransactionRef (furszy) f0c2255 Add mempool.dat to doc/files.md (furszy) 8e52226 Add DumpMempool and LoadMempool (Pieter Wuille) 44c635d Add AcceptToMemoryPoolWithTime function (Pieter Wuille) 6bbc6a9 Add feedelta to TxMempoolInfo (Pieter Wuille) 9979f3d [mempool] move removed and conflicts transaction ref list to vector. (furszy) 4f672c2 Make removed and conflicted arguments optional to remove (Pieter Wuille) e51c4b8 Bypass removeRecursive in removeForReorg (Pieter Wuille) 54cf7c0 Get rid of CTxMempool::lookup() entirely (furszy) 35bc2a9 Finished the switch CTransaction storage in mempool to CTransactionRef and introduced the timeLastMempoolReq coming from btc#8080. (furszy) d10583b An adapted version of btc@b5599147533103efea896a1fc4ff51f2d3ad5808 (furszy) cb4fc6c An adapted version of btc@ed7068302c7490e8061cb3a558a0f83a465beeea (furszy) 9645775 Split up and optimize transaction and block inv queues (furszy) 68bc68f Don't do mempool lookups for "mempool" command without a filter (furszy) 7624823 mapNextTx: use pointer as key, simplify value (furszy) 191c62e Return mempool queries in dependency order (Pieter Wuille) 23c9f3e Eliminate TX trickle bypass, sort TX invs for privacy and priority. (furszy) 6ebfd17 tiny test fix for mempool_tests (Alex Morcos) 8c0016e Check all ancestor state in CTxMemPool::check() (furszy) 91c6096 Add ancestor feerate index to mempool (Suhas Daftuar) 64e84e2 Add ancestor tracking to mempool This implements caching of ancestor state to each mempool entry, similar to descendant tracking, but also including caching sigops-with-ancestors (as that metric will be helpful to future code that implements better transaction selection in CreatenewBlock). (furszy) 8325bb4 Fix mempool limiting for PrioritiseTransaction Redo the feerate index to be based on mining score, rather than fee. (furszy) 1fa40ac Remove work limit in UpdateForDescendants() The work limit served to prevent the descendant walking algorithm from doing too much work by marking the parent transaction as dirty. However to implement ancestor tracking, it's not possible to similarly mark those descendant transactions as dirty without having to calculate them to begin with. This commit removes the work limit altogether. With appropriate chain limits (-limitdescendantcount) the concern about doing too much work inside this function should be mitigated. (furszy) ba32375 Rename CTxMemPool::remove -> removeRecursive remove is no longer called non-recursively, so simplify the logic and eliminate an unnecessary parameter (furszy) c30fa16 CTxMemPool::removeForBlock now uses RemoveStaged (furszy) Pull request description: Ending up 2020 with a large PR :). Included a good number of performance and privacy improvements over the mempool and inv processing/sending areas + added mempool cache dump/load. Almost finishing with #1726 work, getting closer to 9725, and getting closer to be able to decouple the message processing thread <-> wallet and the wallet <-> GUI locks dependencies (which will be another long story.. but well, step by step). The final goal is clear, a much faster syncing process using pivx-qt (a good speed up for pivxd as well), smoother visual navigation when big wallets are syncing, less thread synchronization issues, among all of the improvements that will be coming with the backports adaptations. Adapted the following PRs to our sources: bitcoin#7062 —> only eb30666. bitcoin#7174 —> complete. bitcoin#7562 —> only c5d746a. bitcoin#7594 —> complete. bitcoin#7840 —> complete. bitcoin#7997 —> complete. bitcoin#8080 —> complete bitcoin#8126 —> except e9b4780 (we don't have the mapRelay) and c2a4724 (we don't have the relay expiration vector). bitcoin#8448 —> complete bitcoin#8515 —> complete bitcoin#9408 —> complete bitcoin#9966 —> complete bitcoin#10330 —> complete ACKs for top commit: random-zebra: ACK 9b9c616 Fuzzbawls: ACK 9b9c616 Tree-SHA512: 186bd09bbb19b55ec0d46dc27291663a0df2d60d8238c661a8c9b8cbf3677b6f8a92fa56bd7346a3cb5293712eeccc5d6542ee3c441d35a4f61e7d12e2ce489a

ZIP 239 preparations 3 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#8080 - bitcoin/bitcoin#8082 - bitcoin/bitcoin#8126 - bitcoin/bitcoin#7910 - This is the unsquashed version of bitcoin/bitcoin#8149 - We take three cleanup commits to the protocol / `CInv` code. - bitcoin/bitcoin#8822 - bitcoin/bitcoin#8880 - Excluding the first commit (we don't have the comment it fixes yet). - bitcoin/bitcoin#19322

sipa force-pushed the sharedmempool branch from 532d6f1 to 3a2835a Compare May 30, 2016 16:50

maflcko added Resource usage Refactoring labels May 30, 2016

sipa force-pushed the sharedmempool branch from 3a2835a to 576fa4b Compare May 30, 2016 20:17

sipa force-pushed the sharedmempool branch from 576fa4b to 4042a03 Compare May 30, 2016 20:42

dcousens reviewed May 31, 2016
View reviewed changes

paveljanik reviewed May 31, 2016
View reviewed changes

sipa force-pushed the sharedmempool branch 2 times, most recently from 9ebe8d9 to ec5cc0f Compare May 31, 2016 14:17

sipa force-pushed the sharedmempool branch from ec5cc0f to accda01 Compare May 31, 2016 17:54

sipa force-pushed the sharedmempool branch from accda01 to be3f612 Compare May 31, 2016 18:26

sipa mentioned this pull request Jun 1, 2016

Compact Blocks #8068

Merged

sipa force-pushed the sharedmempool branch from 5bcbc5f to 74a1bb2 Compare June 1, 2016 14:22

paveljanik reviewed Jun 2, 2016
View reviewed changes

sipa added 2 commits June 5, 2016 00:31

Add support for unique_ptr and shared_ptr to memusage

1b9e6d3

Switch CTransaction storage in mempool to std::shared_ptr

8d39d7a

sipa force-pushed the sharedmempool branch from a2f894c to 89ffe94 Compare June 4, 2016 22:31

kazcw reviewed Jun 5, 2016
View reviewed changes

sipa added 3 commits June 6, 2016 02:00

Optimize the relay map to use shared_ptr's

dbfb426

* 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).

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

e9b4780

Optimization: use usec in expiration and reuse nNow

c2a4724

sipa force-pushed the sharedmempool branch from 89ffe94 to c2a4724 Compare June 6, 2016 00:09

laanwj reviewed Jun 7, 2016
View reviewed changes

Get rid of CTxMempool::lookup() entirely

288d85d

laanwj merged commit 288d85d into bitcoin:master Jun 8, 2016

sipa mentioned this pull request Jun 13, 2016

Segregated witness #7910

Closed

7 tasks

furszy mentioned this pull request Jul 3, 2020

Up to CTransactionRef point back ports to do list. PIVX-Project/PIVX#1726

Closed

21 tasks

furszy mentioned this pull request Aug 22, 2020

Make CBlock a vector of shared_ptr of CTransactions PIVX-Project/PIVX#1815

Merged

random-zebra mentioned this pull request Oct 16, 2020

Fix block-connection performance regression and mempool not copying tx performance improvement PIVX-Project/PIVX#1915

Merged

furszy mentioned this pull request Dec 31, 2020

[NET] Inv, mempool and validation improvements PIVX-Project/PIVX#2118

Merged

str4d mentioned this pull request Aug 13, 2021

ZIP 239 preparations 3 zcash/zcash#5274

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

std::shared_ptr based CTransaction storage in mempool #8126

std::shared_ptr based CTransaction storage in mempool #8126

sipa commented May 30, 2016 •

edited

Loading

gmaxwell commented May 30, 2016 •

edited

Loading

paveljanik commented May 30, 2016

sipa commented May 30, 2016

dcousens May 31, 2016

paveljanik May 31, 2016

dcousens May 31, 2016 •

edited

Loading

dcousens May 31, 2016 •

edited

Loading

paveljanik May 31, 2016

dcousens commented May 31, 2016

paveljanik May 31, 2016 •

edited

Loading

sipa commented May 31, 2016

dcousens commented May 31, 2016 •

edited

Loading

sipa commented May 31, 2016 via email

sipa commented May 31, 2016

sipa commented May 31, 2016

sipa commented May 31, 2016

sipa commented May 31, 2016

sipa commented May 31, 2016

dcousens commented Jun 2, 2016

dcousens commented Jun 2, 2016

paveljanik Jun 2, 2016

sipa Jun 4, 2016

gmaxwell commented Jun 4, 2016

gmaxwell commented Jun 4, 2016 •

edited

Loading

sipa commented Jun 4, 2016

kazcw Jun 5, 2016

sipa Jun 6, 2016 •

edited

Loading

laanwj commented Jun 7, 2016

laanwj Jun 7, 2016 •

edited

Loading

sipa Jun 7, 2016

sipa Jun 7, 2016

laanwj commented Jun 7, 2016

std::shared_ptr based CTransaction storage in mempool #8126

std::shared_ptr based CTransaction storage in mempool #8126

Conversation

sipa commented May 30, 2016 • edited Loading

gmaxwell commented May 30, 2016 • edited Loading

paveljanik commented May 30, 2016

sipa commented May 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcousens May 31, 2016 • edited Loading

Choose a reason for hiding this comment

dcousens May 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcousens commented May 31, 2016

paveljanik May 31, 2016 • edited Loading

Choose a reason for hiding this comment

sipa commented May 31, 2016

dcousens commented May 31, 2016 • edited Loading

sipa commented May 31, 2016 via email

sipa commented May 31, 2016

sipa commented May 31, 2016

sipa commented May 31, 2016

sipa commented May 31, 2016

sipa commented May 31, 2016

dcousens commented Jun 2, 2016

dcousens commented Jun 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmaxwell commented Jun 4, 2016

gmaxwell commented Jun 4, 2016 • edited Loading

sipa commented Jun 4, 2016

Choose a reason for hiding this comment

sipa Jun 6, 2016 • edited Loading

Choose a reason for hiding this comment

laanwj commented Jun 7, 2016

laanwj Jun 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj commented Jun 7, 2016

sipa commented May 30, 2016 •

edited

Loading

gmaxwell commented May 30, 2016 •

edited

Loading

dcousens May 31, 2016 •

edited

Loading

dcousens May 31, 2016 •

edited

Loading

paveljanik May 31, 2016 •

edited

Loading

dcousens commented May 31, 2016 •

edited

Loading

gmaxwell commented Jun 4, 2016 •

edited

Loading

sipa Jun 6, 2016 •

edited

Loading

laanwj Jun 7, 2016 •

edited

Loading