Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace mapNextTx with slimmer setSpends #7997

Merged
merged 1 commit into from Jun 2, 2016

Conversation

@kazcw
Copy link
Contributor

commented May 3, 2016

mapNextTx entries comprise about 15% of application memory usage once the mempool
warms up; most of this space is taken by the key type, which contains a uint256.
The same uint256 is accessible from the map's value type.

setSpends represents the information as a set of CSpendRefs. A CSpendRef is,
logically, a reference to a COutPoint; the COutPoint may be either "free" (as in
the case of setSpends.find(COutPoint(...))), or in an element of the vin of a
CTransaction (as in all the values inserted into setSpend). If the COutPoint is
associated with a spending CTransaction, the CSpendRef is also capable of
providing access to the corresponding transaction.

[This object bimodality is necessary because lookup on a set requires
constructing an object of the set's key type, an inconvenience corrected in
C++14.]

Saves about 10% of total memory usage (an entry in mapNextTx is 48 bytes; an
entry in setSpends in 16 bytes). Since the mempool is DynamicUsage-regulated,
this will translate to a larger mempool in the same amount of space.

(Supersedes #7991, which addressed the same issue less efficiently)

@sdaftuar

This comment has been minimized.

Copy link
Member

commented May 5, 2016

Code review ACK, will test.

@dcousens

This comment has been minimized.

Copy link
Contributor

commented May 6, 2016

[This object bimodality is necessary because lookup on a set requires
constructing an object of the set's key type, an inconvenience corrected in
C++14.]

I thought we were only at C++11? Or are you suggesting this is something else that would be corrected by moving to C++14?

@kazcw

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2016

I'm just blaming C++<=11 for my tagged union. When we eventually switch to C++14 there's a much cleaner solution that doesn't require putting two things that should be different types in the same class.

@dcousens

View changes

src/txmempool.h Outdated
return haveSpender ? GetInPoint().GetOutPoint() : *spent;
}
constexpr CInPoint GetInPoint() const noexcept {
return haveSpender ? CInPoint{ spender, n } : throw std::logic_error("!haveSpender in CSpendRef::GetInPoint()");

This comment has been minimized.

Copy link
@dcousens

dcousens May 6, 2016

Contributor

noexcept, then a throw

Did I misunderstand the meaning of this qualifier?

This comment has been minimized.

Copy link
@kazcw

kazcw May 6, 2016

Author Contributor

noexcept guarantees std::terminate occurs if an exception escapes the function; it's a hard assert that's allowed inside a constexpr.

This comment has been minimized.

Copy link
@dcousens

dcousens May 6, 2016

Contributor

TIL, thanks @kazcw

@sdaftuar

This comment has been minimized.

Copy link
Member

commented May 6, 2016

ACK. Nice improvement!

@dcousens

This comment has been minimized.

Copy link
Contributor

commented May 7, 2016

utACK 2642766

@sipa

This comment has been minimized.

Copy link
Member

commented May 9, 2016

Concept ACK

@kazcw

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2016

I just realized that no users of mapNextTx need to know which CTxIn of a transaction spends a particular prevout. This makes a map-based approach possible with the same memory usage as this set implementation. I'm going to replace this with the map implementation because it's definitely cleaner that way (doesn't require the CSpendRef trick, and a map better reflects the function of the data structure).

@kazcw kazcw force-pushed the kazcw:setSpends branch May 9, 2016

@TheBlueMatt

View changes

src/indirectmap.h Outdated
typedef typename base::size_type size_type;

// passthrough (pointer interface)
T& operator[](const K* key) { return m[key]; }

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt May 15, 2016

Contributor

Feel like this might lead to errors at some point? I'd kinda prefer if you only do this for insert().

This comment has been minimized.

Copy link
@sipa

sipa May 16, 2016

Member

I guess it's the correct thing to do if you want to mimic the std::map interface as good as possible, but operator[] automatically constructing elements is annoying...

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented May 15, 2016

ACK 507437a0f067dd696c1fe9698f4ad60a3801ce3f +/- removing [] and forcing insertion through insert(). It'd also be nice to get a test for indirectmap, or at least the following, if you're feeling lazy:

diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 2f867c8..42c3da4 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -673,6 +673,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
             // Check whether its inputs are marked in mapNextTx.
             auto it3 = mapNextTx.find(txin.prevout);
             assert(it3 != mapNextTx.end());
+            assert(it3->first == &txin.prevout);
             assert(it3->second == &tx);
             i++;
         }
@kazcw

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2016

The find-and-maybe-insert operator does seem especially squirrelly in this case. Added a squashme with insert(...) instead; added the assertion.

@sipa

This comment has been minimized.

Copy link
Member

commented May 16, 2016

utACK f379416127f0832ba4aa73c1af874df79b58124c

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented May 17, 2016

Seems you forgot to add the assertion?

@kazcw

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2016

Oops, I hadn't saved the changes to the file yet

@kazcw kazcw force-pushed the kazcw:setSpends branch May 17, 2016

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented May 17, 2016

re-ACK 64949c7f9c7d47df170fa3814bb5aa89235c8e74

@sipa

This comment has been minimized.

Copy link
Member

commented Jun 2, 2016

Can you squash?

mapNextTx: use pointer as key, simplify value
Saves about 10% of application memory usage once the mempool warms up. Since the
mempool is DynamicUsage-regulated, this will translate to a larger mempool in
the same amount of space.

Map value type: eliminate the vin index; no users of the map need to know which
input of the transaction is spending the prevout.

Map key type: replace the COutPoint with a pointer to a COutPoint. A COutPoint
is 36 bytes, but each COutPoint is accessible from the same map entry's value.
A trivial DereferencingComparator functor allows indirect map keys, but the
resulting syntax is misleading: `map.find(&outpoint)`. Implement an indirectmap
that acts as a wrapper to a map that uses a DereferencingComparator, supporting
a syntax that accurately reflect the container's semantics: inserts and
iterators use pointers since they store pointers and need them to remain
constant and dereferenceable, but lookup functions take const references.

@kazcw kazcw force-pushed the kazcw:setSpends branch to 9805f4a Jun 2, 2016

@kazcw

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2016

Squashed all my commits from 64949c7 -> 9805f4a

@sipa

This comment has been minimized.

Copy link
Member

commented Jun 2, 2016

utACK 9805f4a, but well-tested by the CTxMemPool::check function.

@sipa sipa merged commit 9805f4a into bitcoin:master Jun 2, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
sipa added a commit that referenced this pull request Jun 2, 2016
Merge #7997: replace mapNextTx with slimmer setSpends
9805f4a mapNextTx: use pointer as key, simplify value (Kaz Wesley)
codablock added a commit to codablock/dash that referenced this pull request Dec 22, 2017
Merge bitcoin#7997: replace mapNextTx with slimmer setSpends
9805f4a mapNextTx: use pointer as key, simplify value (Kaz Wesley)
andvgal added a commit to energicryptocurrency/energi that referenced this pull request Jan 6, 2019
Merge bitcoin#7997: replace mapNextTx with slimmer setSpends
9805f4a mapNextTx: use pointer as key, simplify value (Kaz Wesley)
@LitecoinZ LitecoinZ referenced this pull request Apr 30, 2019
53 of 77 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.