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. sipa cross-referenced this on Nov 13, 2016 from issue Make CTransaction actually immutable by sipa
  9. gmaxwell commented at 5:57 PM on November 14, 2016: contributor

    tested ACK.

  10. sipa force-pushed on Nov 15, 2016
  11. 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).

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

  13. 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:

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

    Best shared_ptr run:

    2016-11-16 10:17:40   - Load block from disk: 1.61ms [283.93s]
2016-11-16 10:17:40     - Sanity checks: 0.18ms [76.82s]
2016-11-16 10:17:40     - Fork checks: 0.03ms [83.22s]
2016-11-16 10:17:40       - Connect 64 transactions: 17.43ms (0.272ms/tx, 0.032ms/txin) [717.76s]
2016-11-16 10:17:40     - Verify 550 txins: 17.47ms (0.032ms/txin) [724.51s]
2016-11-16 10:17:40     - Index writing: 0.02ms [5.34s]
2016-11-16 10:17:40     - Callbacks: 0.02ms [3.85s]
2016-11-16 10:17:40   - Connect total: 17.81ms [911.36s]
2016-11-16 10:17:40   - Flush: 0.40ms [87.91s]
2016-11-16 10:17:40   - Writing chainstate: 0.03ms [112.31s]
2016-11-16 10:17:40   - Connect postprocess: 0.08ms [25.61s]
2016-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.

  14. laanwj cross-referenced this on Nov 17, 2016 from issue net: Decouple CConnman and message serialization by theuni
  15. in src/serialize.h:None 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: 
    namespace deserialize
{
  struct tag {};
  constexpr tag do;
}

    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 12:42 AM on November 18, 2016:

    Fixed by using deserialize_type instead of deserialize_t.

  16. in src/primitives/transaction.h:None 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.

  17. in src/core_memusage.h:None 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 12:42 AM on November 18, 2016:

    Fixed.

  18. in src/main.cpp:None 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.

  19. in src/main.cpp:None 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 12:33 AM on November 18, 2016:

    You take one down, pass it around. Infinity cases of nits in the code.

  20. in src/main.cpp:None 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 12:34 AM on November 18, 2016:

    Not going to touch that now, as I believe #9014 will rewrite this anyway.

  21. in src/main.cpp:None 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 12:35 AM on November 18, 2016:

    Good point, but let's do that as an independent improvement.

  22. in src/txmempool.cpp:None 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 12:40 AM on November 18, 2016:

    Likewise.

  23. theuni approved
  24. theuni commented at 9:05 PM on November 17, 2016: member

    utACK. Several nits, but nothing worth blocking.

  25. in src/primitives/transaction.h:None 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:

    template <typename T>
static inline CTransactionRef MakeTransactionRef(T&& txIn)
{
  return std::make_shared<const CTransaction>(std::forward<T>(txIn));
}
    TheBlueMatt commented at 12:38 AM on November 20, 2016:

    Might still be worth replacing the 6 functions with 3 using @theuni's suggestion

  26. in src/core_memusage.h:None 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

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

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

  29. jtimon commented at 11:44 PM on November 17, 2016: contributor

    utACK 9bc6cbc besides some nits by @theuni

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

    CTransactionRef a(MakeTransactionRef());
CTransactionRef b(a);
CTransactionRef c(std::move(b));

    ?

  31. sipa commented at 12: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.

  32. sipa force-pushed on Nov 18, 2016
  33. sipa commented at 12: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.
  34. 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):

  35. in src/blockencodings.h:None 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.

  36. in src/serialize.h:None 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 12:25 AM on November 20, 2016:

    nit: comment out of date

    sipa commented at 2:01 AM on November 20, 2016:

    Fixed.

  37. TheBlueMatt commented at 12:52 AM on November 20, 2016: contributor

    utACK 39e832c90012db03822f61009060712a47f7f81d...comments all dont matter so much

  38. Add serialization for unique_ptr and shared_ptr 0e85204a10
  39. Add deserializing constructors to CTransaction and CMutableTransaction da60506fc8
  40. Make CBlock::vtx a vector of shared_ptr<CTransaction> 1662b437b3
  41. Introduce convenience type CTransactionRef b4e4ba475a
  42. sipa force-pushed on Nov 20, 2016
  43. sipa commented at 2:01 AM on November 20, 2016: member

    Addressed all of @TheBlueMatt's nits.

  44. laanwj merged this on Nov 21, 2016
  45. laanwj closed this on Nov 21, 2016
  46. laanwj commented at 9:52 AM on November 21, 2016: member

    Tested ACK b4e4ba4

  47. laanwj referenced this in commit 7490ae8b69 on Nov 21, 2016
  48. TheBlueMatt cross-referenced this on Nov 22, 2016 from issue Fix block-connection performance regression by TheBlueMatt
  49. laanwj cross-referenced this on Dec 2, 2016 from issue 0.13.2 backports by laanwj
  50. codablock referenced this in commit f12610c030 on Jan 15, 2018
  51. gladcow cross-referenced this on Mar 1, 2018 from issue Backport compact blocks functionality from bitcoin by gladcow
  52. gladcow referenced this in commit 89f536b8e1 on Mar 5, 2018
  53. gladcow referenced this in commit e02c4b0aca on Mar 13, 2018
  54. gladcow referenced this in commit d329b9f02b on Mar 14, 2018
  55. gladcow referenced this in commit 2ac27b41bf on Mar 15, 2018
  56. gladcow referenced this in commit 583b4aef6b on Mar 15, 2018
  57. gladcow referenced this in commit caaad1db64 on Mar 15, 2018
  58. gladcow referenced this in commit d1c84250d6 on Mar 15, 2018
  59. gladcow referenced this in commit 6eb5240754 on Mar 24, 2018
  60. gladcow referenced this in commit a96ed5874d on Apr 4, 2018
  61. UdjinM6 referenced this in commit bc45a2f87a on Apr 11, 2018
  62. str4d cross-referenced this on Apr 17, 2018 from issue Upstream serialization improvements by str4d
  63. zkbot referenced this in commit 3b68ab255f on Apr 17, 2018
  64. zkbot referenced this in commit d408e23ab7 on Apr 18, 2018
  65. zkbot referenced this in commit 0753a0e8a9 on Apr 19, 2018
  66. 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.

  67. andvgal referenced this in commit 34a1ec95c7 on Jan 6, 2019
  68. andvgal referenced this in commit fd5c50bc2b on Jan 6, 2019
  69. CryptoCentric referenced this in commit 8cfa735b96 on Feb 24, 2019
  70. CryptoCentric referenced this in commit dd3fd51204 on Feb 28, 2019
  71. furszy cross-referenced this on Jul 3, 2020 from issue Up to CTransactionRef point back ports to do list. by furszy
  72. furszy cross-referenced this on Aug 22, 2020 from issue Make CBlock a vector of shared_ptr of CTransactions by furszy
  73. furszy referenced this in commit 7b2b9d048e on Sep 24, 2020
  74. bitcoin locked this on Sep 8, 2021
Contributors
sipatheunigmaxwellsdaftuarjtimonTheBlueMattlaanwjarielgabizon
Labels
Refactoring
Linked (view graph)
#8580 Make CTransaction actually immutable#8589 Inline CTxInWitness inside CTxIn#9014 Fix block-connection performance regression#9128 net: Decouple CConnman and message serialization#9264 0.13.2 backports
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: 2026-05-19 06:55 UTC