Make CBlock a vector of shared_ptr of CTransactions #9125

pull sipa wants to merge 4 commits into bitcoin:master from sipa:sharedblock changing 27 files +261 −171
  1. sipa commented at 1:58 am on November 11, 2016: member

    This is another preparation for #8580, avoiding performance problems and large code changes ahead of time (I was originally planning to do these afterwards, but that would temporarily introduce extra complexity).

    In many places we’ve started using std::shared_ptr<const CTransaction> instead of CTransaction itself, as transactions are shared between many data structures (orphan pool, mempool, relay pool, partially-reconstructed transactions, …), but a notable exception was CBlock, which so far always required a full copy in and out. If CTransaction is to become immutable, that will make things even worse when modifying the coinbase (https://github.com/bitcoin/bitcoin/pull/8580#issuecomment-243052533).

    Advantages:

    • The mining code no longer needs to create a deep copy of all transactions; the resulting CBlockTemplate just shares transactions with the mempool they’re taken from.
    • Compact block reconstruction no longer copies transactions (not from the prefilled structure to the internal partial block, and not from the partial block to the full block).
    • Block processing no longer copies full transactions to txChanged.
    • CTransaction becomes constructable-from-serialization, preparing for #8580.

    Disadvantages:

    • An extra pointer indirection and atomic increments/decrements in some places.
  2. fanquake added the label Refactoring on Nov 11, 2016
  3. theuni commented at 2:36 am on November 11, 2016: member
    Concept ACK. Very curious to see some memory usage numbers :)
  4. sipa commented at 7:12 am on November 11, 2016: member
    I created a branch with some follow-up shared_ptr all the things and other optimizations: https://github.com/sipa/bitcoin/commits/sharedblock2
  5. sipa force-pushed on Nov 11, 2016
  6. sipa force-pushed on Nov 12, 2016
  7. sipa force-pushed on Nov 13, 2016
  8. gmaxwell commented at 5:57 pm on November 14, 2016: contributor
    tested ACK.
  9. sipa force-pushed on Nov 15, 2016
  10. gmaxwell commented at 7:49 am on November 15, 2016: contributor
    FWIW, I attempted to test the performance impact of this but it is a bit difficulty to do a fair comparison. I am reasonably confident that it does not produce a significant slowdown. (logically it should be a speedup, but I don’t think my measurements were fair/reliable enough to determine if there was on or not).
  11. sdaftuar commented at 8:01 pm on November 16, 2016: member
    I did a quick performance comparison running on 2 days of historical data, and though I was unable to quantify any meaningful performance difference, I agree that this doesn’t appear to cause a significant slowdown.
  12. sipa commented at 8:07 pm on November 16, 2016: member

    Benchmark results (25 reindex-chainstates until last checkpoint, default dbcache, otherwise unloaded 24-core machine, measured by -debug=bench) on both master and #9125+#8580+#8589+a few shared_ptr optimizations I haven’t PR’ed yet:

    • Average: master 1490s, shared_ptr 1442s

    Best master run:

     02016-11-16 12:27:14   - Load block from disk: 1.25ms [273.57s]
 12016-11-16 12:27:14     - Sanity checks: 0.14ms [70.95s]
 22016-11-16 12:27:14     - Fork checks: 0.02ms [80.76s]
 32016-11-16 12:27:14       - Connect 64 transactions: 11.06ms (0.173ms/tx, 0.020ms/txin) [711.67s]
 42016-11-16 12:27:14     - Verify 550 txins: 11.09ms (0.020ms/txin) [718.24s]
 52016-11-16 12:27:14     - Index writing: 0.03ms [5.25s]
 62016-11-16 12:27:14     - Callbacks: 0.01ms [3.75s]
 72016-11-16 12:27:14   - Connect total: 11.34ms [896.52s]
 82016-11-16 12:27:14   - Flush: 0.26ms [86.70s]
 92016-11-16 12:27:14   - Writing chainstate: 0.02ms [112.45s]
102016-11-16 12:27:14   - Connect postprocess: 0.42ms [86.88s]
112016-11-16 12:27:14 - Connect block: 13.30ms [1456.12s]

    Best shared_ptr run:

     02016-11-16 10:17:40   - Load block from disk: 1.61ms [283.93s]
 12016-11-16 10:17:40     - Sanity checks: 0.18ms [76.82s]
 22016-11-16 10:17:40     - Fork checks: 0.03ms [83.22s]
 32016-11-16 10:17:40       - Connect 64 transactions: 17.43ms (0.272ms/tx, 0.032ms/txin) [717.76s]
 42016-11-16 10:17:40     - Verify 550 txins: 17.47ms (0.032ms/txin) [724.51s]
 52016-11-16 10:17:40     - Index writing: 0.02ms [5.34s]
 62016-11-16 10:17:40     - Callbacks: 0.02ms [3.85s]
 72016-11-16 10:17:40   - Connect total: 17.81ms [911.36s]
 82016-11-16 10:17:40   - Flush: 0.40ms [87.91s]
 92016-11-16 10:17:40   - Writing chainstate: 0.03ms [112.31s]
102016-11-16 10:17:40   - Connect postprocess: 0.08ms [25.61s]
112016-11-16 10:17:40 - Connect block: 19.94ms [1421.12s]

    Going to include just this PR in future benchmarks as well, as there seem to be some slowdowns too, and I don’t know where they’re coming from.

  13. in src/serialize.h: in 9ea07616f2 outdated 
    34+ *
35+ * is a deserializing constructor, which builds the type by
36+ * deserializing it from s. If T contains const fields, this
37+ * is likely the only way to do so.
38+ */
39+struct deserialize_t {};
    theuni commented at 7:26 pm on November 17, 2016:
    Nits: _t is reserved by posix, and maybe namespace “deserialize” to avoid future shadowing oopses?
    sipa commented at 9:11 pm on November 17, 2016:
    I’m not sure what you are suggesting here.
    theuni commented at 10:01 pm on November 17, 2016: 
    0namespace deserialize
1{
2  struct tag {};
3  constexpr tag do;
4}

    I suppose it’s not worry worrying about though. I can’t come up with a case where using a local variable “deserialize” could compile in an unintended way.

    sipa commented at 0:42 am on November 18, 2016:
    Fixed by using deserialize_type instead of deserialize_t.
  14. in src/primitives/transaction.h: in b3a8200c86 outdated 
    378@@ -379,6 +379,7 @@ class CTransaction
379 
380     /** Convert a CMutableTransaction into a CTransaction. */
381     CTransaction(const CMutableTransaction &tx);
382+    CTransaction(CMutableTransaction &&tx);
383 
384     CTransaction& operator=(const CTransaction& tx);
    theuni commented at 7:31 pm on November 17, 2016:
    Does this need a move assignment operator too? Or is this being removed in a follow-up anyway?
    sipa commented at 9:12 pm on November 17, 2016:
    All assignment operators go away in #8580.
  15. in src/core_memusage.h: in 8a671e8a16 outdated 
    68@@ -69,8 +69,8 @@ static inline size_t RecursiveDynamicUsage(const CMutableTransaction& tx) {
69 
70 static inline size_t RecursiveDynamicUsage(const CBlock& block) {
71     size_t mem = memusage::DynamicUsage(block.vtx);
72-    for (std::vector<CTransaction>::const_iterator it = block.vtx.begin(); it != block.vtx.end(); it++) {
73-        mem += RecursiveDynamicUsage(*it);
74+    for (const auto& tx : block.vtx) {
75+        mem += RecursiveDynamicUsage(*tx);
    theuni commented at 8:18 pm on November 17, 2016:
    Need to account for the overhead in the control block?
    sipa commented at 0:42 am on November 18, 2016:
    Fixed.
  16. in src/main.cpp: in 8a671e8a16 outdated 
    1681@@ -1682,9 +1682,9 @@ bool GetTransaction(const uint256 &hash, CTransaction &txOut, const Consensus::P
1682     if (pindexSlow) {
1683         CBlock block;
1684         if (ReadBlockFromDisk(block, pindexSlow, consensusParams)) {
1685-            BOOST_FOREACH(const CTransaction &tx, block.vtx) {
1686-                if (tx.GetHash() == hash) {
1687-                    txOut = tx;
1688+            for (const auto& tx : block.vtx) {
    theuni commented at 8:21 pm on November 17, 2016:
    Future optim (probably done in one of your follow-up branches), this could just return a shared_ptr instead.
  17. in src/main.cpp: in 8a671e8a16 outdated 
    2884@@ -2884,7 +2885,7 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams,
2885     // Update chainActive & related variables.
2886     UpdateTip(pindexNew, chainparams);
2887 
2888-    for(unsigned int i=0; i < pblock->vtx.size(); i++)
2889+    for (unsigned int i=0; i < pblock->vtx.size(); i++)
    theuni commented at 8:34 pm on November 17, 2016:
    Fixing them one at a time, eh? :)
    sipa commented at 0:33 am on November 18, 2016:
    You take one down, pass it around. Infinity cases of nits in the code.
  18. in src/main.cpp: in 8a671e8a16 outdated 
    3116             GetMainSignals().SyncTransaction(*tx, pindexNewTip, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK);
3117         }
3118         // ... and about transactions that got confirmed:
3119-        for(unsigned int i = 0; i < txChanged.size(); i++)
3120-            GetMainSignals().SyncTransaction(std::get<0>(txChanged[i]), std::get<1>(txChanged[i]), std::get<2>(txChanged[i]));
3121+        for (unsigned int i = 0; i < txChanged.size(); i++)
    theuni commented at 8:38 pm on November 17, 2016:
    unrelated nit: using range-for or iterators here would avoid looking up in the vector 3x.
    sipa commented at 0:34 am on November 18, 2016:
    Not going to touch that now, as I believe #9014 will rewrite this anyway.
  19. in src/main.cpp: in 8a671e8a16 outdated 
    3472+                                 strprintf("Transaction check failed (tx hash %s) %s", tx->GetHash().ToString(), state.GetDebugMessage()));
3473 
3474     unsigned int nSigOps = 0;
3475     for (const auto& tx : block.vtx)
3476     {
3477-        nSigOps += GetLegacySigOpCount(tx);
    theuni commented at 8:41 pm on November 17, 2016:
    Nit: This could be combined with the loop above, and &*tx could be cached to avoid duped dereference cost.
    sipa commented at 0:35 am on November 18, 2016:
    Good point, but let’s do that as an independent improvement.
  20. in src/txmempool.cpp: in 8a671e8a16 outdated 
    612         if (i != mapTx.end())
613             entries.push_back(*i);
614     }
615-    BOOST_FOREACH(const CTransaction& tx, vtx)
616+    for (const auto& tx : vtx)
617     {
    theuni commented at 9:03 pm on November 17, 2016:
    nit: could cache the dereferenced tx since it’s used a few times
    sipa commented at 0:40 am on November 18, 2016:
    Likewise.
  21. theuni approved
  22. theuni commented at 9:05 pm on November 17, 2016: member
    utACK. Several nits, but nothing worth blocking.
  23. in src/primitives/transaction.h: in 9bc6cbc4e2 outdated 
    477 
478+typedef std::shared_ptr<const CTransaction> CTransactionRef;
479+static inline CTransactionRef MakeTransactionRef() { return std::make_shared<const CTransaction>(); }
480+static inline CTransactionRef MakeTransactionRef(const CTransaction& txIn) { return std::make_shared<const CTransaction>(txIn); }
481+static inline CTransactionRef MakeTransactionRef(const CMutableTransaction& txIn) { return std::make_shared<const CTransaction>(txIn); }
482+static inline CTransactionRef MakeTransactionRef(CMutableTransaction&& txIn) { return std::make_shared<const CTransaction>(std::move(txIn)); }
    jtimon commented at 10:55 pm on November 17, 2016:

    Where is this version used? I see calls that call std::move before calling that maybe should be using this instead?

    In any case, it doesn’t feel right to have so many versions of the function somehow. Specially one some with & and others with &&

    theuni commented at 11:06 pm on November 17, 2016:

    I believe the overloads could be replaced with:

    0template <typename T>
1static inline CTransactionRef MakeTransactionRef(T&& txIn)
2{
3  return std::make_shared<const CTransaction>(std::forward<T>(txIn));
4}
    TheBlueMatt commented at 0:38 am on November 20, 2016:
    Might still be worth replacing the 6 functions with 3 using @theuni’s suggestion
  24. in src/core_memusage.h: in 9bc6cbc4e2 outdated 
    68@@ -69,8 +69,8 @@ static inline size_t RecursiveDynamicUsage(const CMutableTransaction& tx) {
69 
70 static inline size_t RecursiveDynamicUsage(const CBlock& block) {
71     size_t mem = memusage::DynamicUsage(block.vtx);
72-    for (std::vector<CTransaction>::const_iterator it = block.vtx.begin(); it != block.vtx.end(); it++) {
    jtimon commented at 11:00 pm on November 17, 2016:
    Thanks
  25. sipa commented at 11:09 pm on November 17, 2016: member
    @jtimon I believe all of them are used in successor PRs. @theuni That overload won’t work for the arguments that take a CTransactionRef.
  26. sipa commented at 11:11 pm on November 17, 2016: member
    @jtimon std::move just casts to an T&& argument. It’s to indicate that the called function may destroy the object.
  27. jtimon commented at 11:44 pm on November 17, 2016: contributor
    utACK 9bc6cbc besides some nits by @theuni
  28. theuni commented at 11:52 pm on November 17, 2016: member

    @sipa I’m unsure why you’d want to do that? Why wouldn’t you just

    0CTransactionRef a(MakeTransactionRef());
1CTransactionRef b(a);
2CTransactionRef c(std::move(b));

    ?

  29. sipa commented at 0:03 am on November 18, 2016: member
    @theuni In #8580 I add a templated constructor to CWalletTx and CMerkleTx that just passes its arguments to MakeTransactionRef, avoiding duplicating various kinds of constructors in both.
  30. sipa force-pushed on Nov 18, 2016
  31. sipa commented at 0:52 am on November 18, 2016: member

    Rebased, and addressed two nits by @theuni:

    • Added the memory usage for the control block of the vtx entries in CBlock.
    • Changed deserialize_t to deserialize_type to avoid colliding with the reserved _t suffix.
  32. sipa commented at 7:24 pm on November 19, 2016: member

    Some more benchmarks (fastest of 28 reindexes each, 300 MB dbcache, reindex chainstate up to block 295000):

  33. in src/blockencodings.h: in 39e832c900 outdated 
    192@@ -193,7 +193,7 @@ class CBlockHeaderAndShortTxIDs {
193 
194 class PartiallyDownloadedBlock {
195 protected:
196-    std::vector<std::shared_ptr<const CTransaction> > txn_available;
197+    std::vector<CTransactionRef > txn_available;
    TheBlueMatt commented at 11:56 pm on November 19, 2016:
    Nit: bad search/replace here?
    sipa commented at 2:01 am on November 20, 2016:
    Fixed.
  34. in src/serialize.h: in 39e832c900 outdated 
    25@@ -25,6 +26,20 @@
26 static const unsigned int MAX_SIZE = 0x02000000;
27 
28 /**
29+ * Dummy data type to identify deserializing constructors.
30+ *
31+ * By convention, a constructor of a type T with signature
32+ *
33+ *   template <typename Stream> T::T(deserialize_t, Stream& s)
    TheBlueMatt commented at 0:25 am on November 20, 2016:
    nit: comment out of date
    sipa commented at 2:01 am on November 20, 2016:
    Fixed.
  35. TheBlueMatt commented at 0:52 am on November 20, 2016: member
    utACK 39e832c90012db03822f61009060712a47f7f81d…comments all dont matter so much
  36. Add serialization for unique_ptr and shared_ptr 0e85204a10
  37. Add deserializing constructors to CTransaction and CMutableTransaction da60506fc8
  38. Make CBlock::vtx a vector of shared_ptr<CTransaction> 1662b437b3
  39. Introduce convenience type CTransactionRef b4e4ba475a
  40. sipa force-pushed on Nov 20, 2016
  41. sipa commented at 2:01 am on November 20, 2016: member
    Addressed all of @TheBlueMatt’s nits.
  42. laanwj merged this on Nov 21, 2016
  43. laanwj closed this on Nov 21, 2016
  44. laanwj commented at 9:52 am on November 21, 2016: member
    Tested ACK b4e4ba4
  45. laanwj referenced this in commit 7490ae8b69 on Nov 21, 2016
  46. codablock referenced this in commit f12610c030 on Jan 15, 2018
  47. gladcow referenced this in commit 89f536b8e1 on Mar 5, 2018
  48. gladcow referenced this in commit e02c4b0aca on Mar 13, 2018
  49. gladcow referenced this in commit d329b9f02b on Mar 14, 2018
  50. gladcow referenced this in commit 2ac27b41bf on Mar 15, 2018
  51. gladcow referenced this in commit 583b4aef6b on Mar 15, 2018
  52. gladcow referenced this in commit caaad1db64 on Mar 15, 2018
  53. gladcow referenced this in commit d1c84250d6 on Mar 15, 2018
  54. gladcow referenced this in commit 6eb5240754 on Mar 24, 2018
  55. gladcow referenced this in commit a96ed5874d on Apr 4, 2018
  56. UdjinM6 referenced this in commit bc45a2f87a on Apr 11, 2018
  57. zkbot referenced this in commit 3b68ab255f on Apr 17, 2018
  58. zkbot referenced this in commit d408e23ab7 on Apr 18, 2018
  59. zkbot referenced this in commit 0753a0e8a9 on Apr 19, 2018
  60. in src/primitives/transaction.h:468 in da60506fc8 outdated 
    463@@ -460,6 +464,11 @@ struct CMutableTransaction
464         SerializeTransaction(*this, s, ser_action);
465     }
466 
467+    template <typename Stream>
468+    CMutableTransaction(deserialize_type, Stream& s) {
    arielgabizon commented at 10:14 am on April 20, 2018:
    Right now there’s some code redundancy - the only deserialize_type is deserialize, so it’s not really used - like here. But I guess people think there may be more in the future.
  61. andvgal referenced this in commit 34a1ec95c7 on Jan 6, 2019
  62. andvgal referenced this in commit fd5c50bc2b on Jan 6, 2019
  63. CryptoCentric referenced this in commit 8cfa735b96 on Feb 24, 2019
  64. CryptoCentric referenced this in commit dd3fd51204 on Feb 28, 2019
  65. furszy referenced this in commit 7b2b9d048e on Sep 24, 2020
  66. MarcoFalke locked this on Sep 8, 2021


sipa theuni gmaxwell sdaftuar jtimon TheBlueMatt laanwj arielgabizon

Labels
Refactoring

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me