Structure Packing Optimizations in CTransaction and CTxMemPoolEntry #8330

JeremyRubin · Jul 11, 2016

These commits revise the layout of a few key classes to eliminate padding, eliminating useless memory overhead.

CTxMemPoolEntry is decreased to 184 bytes from 192. CTransaction is decreased to 112 bytes from 120.

Furthermore, it is clarified in CTxMempoolEntry where the extra space is (2 bytes) for future overhead-free use.

The introduction of CTxMemPoolEntryFlags is used to bit-pack the bool flags, making room for 6 more future-flags. I have not performance tested this, but it should not be much. It does not save any space at the moment due to padding, but if more bools are added it would compared to naive.

To finish this work, the following should be done: profiling of access patterns to class fields to guarantee cache-line co-residency, cross architecture padding optimizations.

nit: we mostly have newlines between classes.

What's the purpose of UNUSED_BITS?

Maybe a uint32_t bitmap would be the better choice here?

There's no purpose, just to demarcate that these bits are available for future programmers. A uint32_t would be 4 bytes whereas this struct should be 1 byte.

Sorry.. overlooked that you are using a c++ bitfield. But I guess the struct will still consume 4 bytes.

Bitfields are standard C. The size of the struct will be 1 byte. However, the alignment of the entire CTxMempool object will still be 4, so there will be 3 padding bytes at the end of CTxMempool, I think?

@sipa are you sure of the alignment? On my system it is 8 bytes, which leaves no padding for the CTxMemPoolEntry. Perhaps you're on OSX where size_t is 4 bytes not 8 (I didn't look at OSX).

@JeremyRubin size_t is the size of a pointer, which is 32 bits on 32 bits architectures and 64 bits on 64 bits architectures. You're right that the overall alignment is 8 bytes (on my architecture at least).

It seems the flags field becomes 4 bytes anyway, so there is no padding afterwards.

jonasschnelli · Jul 12, 2016

Concept ACK

sipa · Jul 12, 2016

No, I'm not. I haven't actually tried. But I assume the int before the bitfield is 4-aligned, which would make the end of the bitfield byte be at a multiple of 4 plus 1?

gmaxwell · Dec 5, 2016

Needs rebase. Concept ACK, on the placeholder bitfield, perhaps better to make that a comment?

JeremyRubin · Jan 10, 2017

rebased and addressed feedback

JeremyRubin · Mar 27, 2017

Rebased! (removing priority conflicted with this)

ryanofsky · Mar 27, 2017

ryanofsky reviewed Mar 27, 2017

View changes

Kind of a lonely bitfield now with just one bool. But maybe it will have some more flags later.

yeah, hopefully inspires whoever adds a flag in the future...

In commit "Re-pack the CTxMempoolEntry to be closer to optimally packed":

This isn't used, guessing bad rebase.

yeah... hmm :/

Will fix.

In commit "Re-pack the CTxMempoolEntry to be closer to optimally packed":

Also unused.

In commit "Reorder C{,Mutable}Transaction for better packing":

Seems like you could remove ComputeHash now, it doesn't appear to be called anywhere else.

ryanofsky · Mar 28, 2017

ryanofsky reviewed Mar 28, 2017

View changes

In commit "Re-pack the CTxMempoolEntry to be closer to optimally packed"

Unusual indentation here, maybe reformat.

TheBlueMatt · Jul 11, 2017

TheBlueMatt reviewed Jul 11, 2017

View changes

Ehh, why not just have the single bool spendsCoinbase : 1 here instead of making a struct for it?

It's somewhat vestigial; iirc when I originally wrote this patch last year there was another bool that got removed in a later PR.

The only real remaining reason is to make the code "self documenting" so that when/if someone later adds another flag to mempool (or even adds the flag back), they are encouraged to dtrt rather than adding more memory bloat.

Is there harm in having
bool spendsCoinbase : 1;
bool newFlags : 1;
instead of encapsulating the booleans in a flags struct?

Is that valid c++? If so, o_0

In what way is that different from what you're doing now?

sipa · Jul 12, 2017

This reduces CTransaction from 96 bytes to 88 bytes for me (great), but increases CTxMemPoolEntry from 160 bytes to 168 bytes (not so great). NACK as long as that is the case.

JeremyRubin · Jul 13, 2017

@sipa I rewrote this PR to repack the CTransaction versions via a minimal patch.

I'm not sure what was going on with the CTxMemPoolEntry version, I just think it became stale. At the time I wrote the patch initially they were 192 bytes, so 160 bytes/entry is a good improvement anyways (perhaps from removing priority).

sipa · Jul 13, 2017

utACK 37495e0. Verified with printf("%i\n", sizeof(CTransaction)) before and after that this indeed reduces the size of the type.

TheBlueMatt · Jul 14, 2017

utACK 37495e0

ryanofsky · Jul 25, 2017

ryanofsky reviewed Jul 25, 2017

View changes

utACK 37495e0. Should update outdated PR description though to avoid misleading text in git history.

jonasschnelli commented on an outdated diff Jul 12, 2016

View full outdated diff
src/primitives/transaction.h

@@ -433,15 +433,14 @@ class CTransaction

void UpdateHash() const;

};

-

jonasschnelli Jul 12, 2016
Member

nit: we mostly have newlines between classes.

jonasschnelli added the Refactoring label Jul 12, 2016

JeremyRubin changed the title from WIP: Structure Packing Optimizations in CTransaction and CTxMemPoolEntry to Structure Packing Optimizations in CTransaction and CTxMemPoolEntry Mar 27, 2017

JeremyRubin Reorder C{,Mutable}Transaction for better packing

37495e0

+    +    // Space that is typically used as padding. Commented out so that it doesn't cause extra memory use on different architectures.
+    +    // char PADDING_FREE_SPACE[2];
+    +
+    +    // Bool flags
+    +    struct CTxMemPoolEntryFlags {
+    +        int UNUSED_BITS : 6;

-    -    bool spendsCoinbase;       //!< keep track of transactions that spend a coinbase
+    +    struct CTxMemPoolEntryFlags {
+    +        //int UNUSED_BITS : 7;
+    +        bool spendsCoinbase : 1;       //!< keep track of transactions that spend a coinbase

@@ -94,7 +89,17 @@ class CTxMemPoolEntry
          uint64_t nSizeWithAncestors;
          CAmount nModFeesWithAncestors;
          int64_t nSigOpCostWithAncestors;
+    +    size_t nTxWeight;          //!< ... and avoid recomputing tx weight (also used for GetTxSize())
+    +    size_t nModSize;           //!< ... and modified size for priority

@@ -76,9 +76,12 @@ uint256 CTransaction::GetWitnessHash() const
      }
      /* For backward compatibility, the hash is initialized to 0. TODO: remove the need for this default constructor entirely. */
-    -CTransaction::CTransaction() : nVersion(CTransaction::CURRENT_VERSION), vin(), vout(), nLockTime(0), hash() {}
-    -CTransaction::CTransaction(const CMutableTransaction &tx) : nVersion(tx.nVersion), vin(tx.vin), vout(tx.vout), nLockTime(tx.nLockTime), hash(ComputeHash()) {}

-    -    tx(_tx), nFee(_nFee), nTime(_nTime), entryHeight(_entryHeight),
-    -    spendsCoinbase(_spendsCoinbase), sigOpCost(_sigOpsCost), lockPoints(lp)
+    +                                 bool _spendsCoinbase, int64_t _sigOpsCost, LockPoints lp):
+    +lockPoints(lp), tx(_tx), nFee(_nFee)

bitcoin/bitcoin

Structure Packing Optimizations in CTransaction and CTxMemPoolEntry #8330

JeremyRubin commented Jul 11, 2016

jonasschnelli commented on an outdated diff Jul 12, 2016

jonasschnelli and 2 others commented on an outdated diff Jul 12, 2016

jonasschnelli added the Refactoring label Jul 12, 2016

jonasschnelli commented Jul 12, 2016

sipa commented Jul 12, 2016

gmaxwell commented Dec 5, 2016

JeremyRubin commented Jan 10, 2017 • edited

JeremyRubin changed the title from WIP: Structure Packing Optimizations in CTransaction and CTxMemPoolEntry to Structure Packing Optimizations in CTransaction and CTxMemPoolEntry Mar 27, 2017

JeremyRubin commented Mar 27, 2017

ryanofsky reviewed Mar 27, 2017

View changes

ryanofsky reviewed Mar 28, 2017

View changes

TheBlueMatt reviewed Jul 11, 2017

View changes

sipa commented Jul 12, 2017 • edited

JeremyRubin commented Jul 13, 2017

sipa commented Jul 13, 2017

TheBlueMatt commented Jul 14, 2017

ryanofsky reviewed Jul 25, 2017

View changes

bitcoin/bitcoin

Join GitHub today

Structure Packing Optimizations in CTransaction and CTxMemPoolEntry #8330

Conversation

JeremyRubin commented Jul 11, 2016

Show 1 comment Hide comment jonasschnelli commented on an outdated diff Jul 12, 2016

Show 6 comments Hide comments jonasschnelli and 2 others commented on an outdated diff Jul 12, 2016

sipa via email Jul 12, 2016

jonasschnelli added the Refactoring label Jul 12, 2016

jonasschnelli commented Jul 12, 2016

sipa commented Jul 12, 2016

gmaxwell commented Dec 5, 2016

JeremyRubin commented Jan 10, 2017 • edited

JeremyRubin changed the title from WIP: Structure Packing Optimizations in CTransaction and CTxMemPoolEntry to Structure Packing Optimizations in CTransaction and CTxMemPoolEntry Mar 27, 2017

JeremyRubin commented Mar 27, 2017

ryanofsky reviewed Mar 27, 2017 View changes

ryanofsky reviewed Mar 28, 2017 View changes

TheBlueMatt reviewed Jul 11, 2017 View changes

sipa commented Jul 12, 2017 • edited

JeremyRubin commented Jul 13, 2017

sipa commented Jul 13, 2017

TheBlueMatt commented Jul 14, 2017

ryanofsky reviewed Jul 25, 2017 View changes

jonasschnelli commented on an outdated diff Jul 12, 2016

jonasschnelli and 2 others commented on an outdated diff Jul 12, 2016

ryanofsky reviewed Mar 27, 2017

View changes

ryanofsky reviewed Mar 28, 2017

View changes

TheBlueMatt reviewed Jul 11, 2017

View changes

ryanofsky reviewed Jul 25, 2017

View changes