Skip to content

Commit

Permalink
Reduce CCoinsMap::value_type from 96 to 88 bytes
Browse files Browse the repository at this point in the history 
This PR is similar to bitcoin#17060, but without the incorrect alignment, and
with safeguards. CCoinsCacheEntry makes use of Coin's padding to store
the flag.

Unfortunately great care needs to be taken with this approach that the
padding is not accidentally overwritten, e.g. when assigning/moving a coin.

This PR solves this issue by chainging CCoinsCacheEntry with getters,
and having only a single MutableCoin() method that safely allows to modify
the coin member without risking the risk over accidentally overwriting
the padding used to store the flags.

The original struct layout was like this:

    96 CCoinsMap::value_type
        36 COutPoint
            32 uint256
            4 uint32_t
        4 >>> PADDING <<<
        56 CCoinsCacheEntry
            48 Coin
                40 CTxOut
                    8 nValue
                    32 CScript
                4 fCoinBase & nHeight
                4 >>> PADDING <<<
            1 flags (dirty & fresh)
            7 >>> PADDING <<<

After that PR the struct layout looks like this:

    88 CCoinsMap::value_type
        36 COutPoint
            32 uint256
            4 uint32_t
        4 >>> PADDING <<<
        48 CCoinsCacheEntry
            48 Coin
                40 CTxOut
                    8 nValue
                    32 CScript
                4 fCoinBase & nHeight
                4 padding: stores flags (dirty & fresh)

That change should translate to ~8% less memory usage of the CCoinsMap.
  • Loading branch information
@martinus
martinus committed Aug 14, 2021
1 parent adf9bcf commit a6dc21e
Show file tree
Hide file tree
Showing 5 changed files with 136 additions and 56 deletions.
78 changes: 44 additions & 34 deletions src/coins.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,19 +45,19 @@ CCoinsMap::iterator CCoinsViewCache::FetchCoin(const COutPoint &outpoint) const
if (!base->GetCoin(outpoint, tmp))
return cacheCoins.end();
CCoinsMap::iterator ret = cacheCoins.emplace(std::piecewise_construct, std::forward_as_tuple(outpoint), std::forward_as_tuple(std::move(tmp))).first;
if (ret->second.coin.IsSpent()) {
if (ret->second.GetCoin().IsSpent()) {
// The parent only has an empty entry for this outpoint; we can consider our
// version as fresh.
ret->second.flags = CCoinsCacheEntry::FRESH;
ret->second.Flags(CCoinsCacheEntry::FRESH);
}
cachedCoinsUsage += ret->second.coin.DynamicMemoryUsage();
cachedCoinsUsage += ret->second.GetCoin().DynamicMemoryUsage();
return ret;
}

bool CCoinsViewCache::GetCoin(const COutPoint &outpoint, Coin &coin) const {
CCoinsMap::const_iterator it = FetchCoin(outpoint);
if (it != cacheCoins.end()) {
coin = it->second.coin;
coin = it->second.GetCoin();
return !coin.IsSpent();
}
return false;
Expand All @@ -71,10 +71,10 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
std::tie(it, inserted) = cacheCoins.emplace(std::piecewise_construct, std::forward_as_tuple(outpoint), std::tuple<>());
bool fresh = false;
if (!inserted) {
cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
cachedCoinsUsage -= it->second.GetCoin().DynamicMemoryUsage();
}
if (!possible_overwrite) {
if (!it->second.coin.IsSpent()) {
if (!it->second.GetCoin().IsSpent()) {
throw std::logic_error("Attempted to overwrite an unspent coin (when possible_overwrite is false)");
}
// If the coin exists in this cache as a spent coin and is DIRTY, then
Expand All @@ -90,11 +90,11 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
//
// If the coin doesn't exist in the current cache, or is spent but not
// DIRTY, then it can be marked FRESH.
fresh = !(it->second.flags & CCoinsCacheEntry::DIRTY);
fresh = !(it->second.Flags() & CCoinsCacheEntry::DIRTY);
}
it->second.coin = std::move(coin);
it->second.flags |= CCoinsCacheEntry::DIRTY | (fresh ? CCoinsCacheEntry::FRESH : 0);
cachedCoinsUsage += it->second.coin.DynamicMemoryUsage();
it->second.MutableCoin([&](Coin& c) { c = std::move(coin); });
it->second.Flags(it->second.Flags() | CCoinsCacheEntry::DIRTY | (fresh ? CCoinsCacheEntry::FRESH : 0));
cachedCoinsUsage += it->second.GetCoin().DynamicMemoryUsage();
}

void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) {
Expand All @@ -119,15 +119,15 @@ void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool
bool CCoinsViewCache::SpendCoin(const COutPoint &outpoint, Coin* moveout) {
CCoinsMap::iterator it = FetchCoin(outpoint);
if (it == cacheCoins.end()) return false;
cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
cachedCoinsUsage -= it->second.GetCoin().DynamicMemoryUsage();
if (moveout) {
*moveout = std::move(it->second.coin);
it->second.MutableCoin([&](Coin& c) { *moveout = std::move(c); });
}
if (it->second.flags & CCoinsCacheEntry::FRESH) {
if (it->second.Flags() & CCoinsCacheEntry::FRESH) {
cacheCoins.erase(it);
} else {
it->second.flags |= CCoinsCacheEntry::DIRTY;
it->second.coin.Clear();
it->second.Flags(it->second.Flags() | CCoinsCacheEntry::DIRTY);
it->second.MutableCoin([&](Coin& c) { c.Clear(); });
}
return true;
}
Expand All @@ -139,18 +139,18 @@ const Coin& CCoinsViewCache::AccessCoin(const COutPoint &outpoint) const {
if (it == cacheCoins.end()) {
return coinEmpty;
} else {
return it->second.coin;
return it->second.GetCoin();
}
}

bool CCoinsViewCache::HaveCoin(const COutPoint &outpoint) const {
CCoinsMap::const_iterator it = FetchCoin(outpoint);
return (it != cacheCoins.end() && !it->second.coin.IsSpent());
return (it != cacheCoins.end() && !it->second.GetCoin().IsSpent());
}

bool CCoinsViewCache::HaveCoinInCache(const COutPoint &outpoint) const {
CCoinsMap::const_iterator it = cacheCoins.find(outpoint);
return (it != cacheCoins.end() && !it->second.coin.IsSpent());
return (it != cacheCoins.end() && !it->second.GetCoin().IsSpent());
}

uint256 CCoinsViewCache::GetBestBlock() const {
Expand All @@ -166,48 +166,58 @@ void CCoinsViewCache::SetBestBlock(const uint256 &hashBlockIn) {
bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn) {
for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); it = mapCoins.erase(it)) {
// Ignore non-dirty entries (optimization).
if (!(it->second.flags & CCoinsCacheEntry::DIRTY)) {
if (!(it->second.Flags() & CCoinsCacheEntry::DIRTY)) {
continue;
}
CCoinsMap::iterator itUs = cacheCoins.find(it->first);
if (itUs == cacheCoins.end()) {
// The parent cache does not have an entry, while the child cache does.
// We can ignore it if it's both spent and FRESH in the child
if (!(it->second.flags & CCoinsCacheEntry::FRESH && it->second.coin.IsSpent())) {
if (!(it->second.Flags() & CCoinsCacheEntry::FRESH && it->second.GetCoin().IsSpent())) {
// Create the coin in the parent cache, move the data up
// and mark it as dirty.
CCoinsCacheEntry& entry = cacheCoins[it->first];
entry.coin = std::move(it->second.coin);
cachedCoinsUsage += entry.coin.DynamicMemoryUsage();
entry.flags = CCoinsCacheEntry::DIRTY;
entry.MutableCoin([&](Coin& entryCoin) {
it->second.MutableCoin([&](Coin& itCoin) {
entryCoin = std::move(itCoin);
});
});
cachedCoinsUsage += entry.GetCoin().DynamicMemoryUsage();
entry.Flags(CCoinsCacheEntry::DIRTY);
// We can mark it FRESH in the parent if it was FRESH in the child
// Otherwise it might have just been flushed from the parent's cache
// and already exist in the grandparent
if (it->second.flags & CCoinsCacheEntry::FRESH) {
entry.flags |= CCoinsCacheEntry::FRESH;
if (it->second.Flags() & CCoinsCacheEntry::FRESH) {
entry.Flags(entry.Flags() | CCoinsCacheEntry::FRESH);
}
}
} else {
// Found the entry in the parent cache
if ((it->second.flags & CCoinsCacheEntry::FRESH) && !itUs->second.coin.IsSpent()) {
if ((it->second.Flags() & CCoinsCacheEntry::FRESH) && !itUs->second.GetCoin().IsSpent()) {
// The coin was marked FRESH in the child cache, but the coin
// exists in the parent cache. If this ever happens, it means
// the FRESH flag was misapplied and there is a logic error in
// the calling code.
throw std::logic_error("FRESH flag misapplied to coin that exists in parent cache");
}

if ((itUs->second.flags & CCoinsCacheEntry::FRESH) && it->second.coin.IsSpent()) {
if ((itUs->second.Flags() & CCoinsCacheEntry::FRESH) && it->second.GetCoin().IsSpent()) {
// The grandparent cache does not have an entry, and the coin
// has been spent. We can just delete it from the parent cache.
cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
cachedCoinsUsage -= itUs->second.GetCoin().DynamicMemoryUsage();
cacheCoins.erase(itUs);
} else {
// A normal modification.
cachedCoinsUsage -= itUs->second.coin.DynamicMemoryUsage();
itUs->second.coin = std::move(it->second.coin);
cachedCoinsUsage += itUs->second.coin.DynamicMemoryUsage();
itUs->second.flags |= CCoinsCacheEntry::DIRTY;
cachedCoinsUsage -= itUs->second.GetCoin().DynamicMemoryUsage();

itUs->second.MutableCoin([&](Coin& itUsCoin) {
it->second.MutableCoin([&](Coin& itCoin) {
itUsCoin = std::move(itCoin);
});
});

cachedCoinsUsage += itUs->second.GetCoin().DynamicMemoryUsage();
itUs->second.Flags(itUs->second.Flags() | CCoinsCacheEntry::DIRTY);
// NOTE: It isn't safe to mark the coin as FRESH in the parent
// cache. If it already existed and was spent in the parent
// cache then marking it FRESH would prevent that spentness
Expand All @@ -229,8 +239,8 @@ bool CCoinsViewCache::Flush() {
void CCoinsViewCache::Uncache(const COutPoint& hash)
{
CCoinsMap::iterator it = cacheCoins.find(hash);
if (it != cacheCoins.end() && it->second.flags == 0) {
cachedCoinsUsage -= it->second.coin.DynamicMemoryUsage();
if (it != cacheCoins.end() && it->second.Flags() == 0) {
cachedCoinsUsage -= it->second.GetCoin().DynamicMemoryUsage();
cacheCoins.erase(it);
}
}
Expand Down
74 changes: 68 additions & 6 deletions src/coins.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
#include <functional>
#include <unordered_map>

class CCoinsCacheEntry;

/**
* A UTXO entry.
*
Expand Down Expand Up @@ -83,6 +85,15 @@ class Coin
size_t DynamicMemoryUsage() const {
return memusage::DynamicUsage(out.scriptPubKey);
}

private:
/**
* Coin is 8byte aligned due to CTxOut's CAmount nValue member. We can expose the required padding here for friend's to use.
* That must be done very carefully though so it is never accidentally overwritten (e.g. when moving/copying the object).
*/
uint32_t padding{};

friend struct CCoinsCacheEntry;
};

/**
Expand All @@ -99,12 +110,27 @@ class Coin
* - unspent, not FRESH, not DIRTY (e.g. an unspent coin fetched from the parent cache)
* - spent, FRESH, not DIRTY (e.g. a spent coin fetched from the parent cache)
* - spent, not FRESH, DIRTY (e.g. a coin is spent and spentness needs to be flushed to the parent)
*
* To save space this class makes use of Coin's internal padding. This must be done carefully though,
* so the only way to modify the m_coin member is through the MutableCoin method which always backs up
* and restores the flags so it can't be overwritten accidentally.
*/
struct CCoinsCacheEntry
class CCoinsCacheEntry
{
Coin coin; // The actual cached data.
unsigned char flags;
Coin m_coin; // The actual cached data.

/**
* RAII object that stores flags on construction and restores it in the dtor.
*/
struct FlagsRestorer {
CCoinsCacheEntry* m_coins_cache_entry{};
unsigned char m_flags{};

explicit FlagsRestorer(CCoinsCacheEntry* coins_cache_entry) : m_coins_cache_entry(coins_cache_entry), m_flags(coins_cache_entry->Flags()) {}
~FlagsRestorer() { m_coins_cache_entry->Flags(m_flags); }
};

public:
enum Flags {
/**
* DIRTY means the CCoinsCacheEntry is potentially different from the
Expand All @@ -126,9 +152,45 @@ struct CCoinsCacheEntry
FRESH = (1 << 1),
};

CCoinsCacheEntry() : flags(0) {}
explicit CCoinsCacheEntry(Coin&& coin_) : coin(std::move(coin_)), flags(0) {}
CCoinsCacheEntry(Coin&& coin_, unsigned char flag) : coin(std::move(coin_)), flags(flag) {}
CCoinsCacheEntry()
{
Flags(0);
}

explicit CCoinsCacheEntry(Coin&& coin) : m_coin(std::move(coin))
{
Flags(0);
}

CCoinsCacheEntry(Coin&& coin, unsigned char flag) : m_coin(std::move(coin))
{
Flags(flag);
}

unsigned char Flags() const noexcept {
return m_coin.padding;
}

void Flags(unsigned char f) noexcept {
m_coin.padding = f;
}

Coin const& GetCoin() const noexcept {
return m_coin;
}

/**
* Because we make use of the coin's padding, we have to be very careful when modifying it.
* Thus, this is the only way to actually modify the coin, and it backs up & restores the flags.
*
* @param op Functor to call with the mutable coin.
*/
template <typename Op>
auto MutableCoin(Op op) {
// using an RAII object to restore flags so it is also restored in case of an exception in op().
auto flagsRestorer = FlagsRestorer(this);
return op(m_coin);
}
};

typedef std::unordered_map<COutPoint, CCoinsCacheEntry, SaltedOutpointHasher> CCoinsMap;
Expand Down
28 changes: 18 additions & 10 deletions src/test/coins_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@

#include <boost/test/unit_test.hpp>

/**
* Make sure we can use the Coin's padding in CCoinsCacheEntry
*/
static_assert(sizeof(CCoinsCacheEntry) == sizeof(Coin));

int ApplyTxInUndo(Coin&& undo, CCoinsViewCache& view, const COutPoint& out);
void UpdateCoins(const CTransaction& tx, CCoinsViewCache& inputs, CTxUndo &txundo, int nHeight);

Expand Down Expand Up @@ -57,10 +62,10 @@ class CCoinsViewTest : public CCoinsView
bool BatchWrite(CCoinsMap& mapCoins, const uint256& hashBlock) override
{
for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end(); ) {
if (it->second.flags & CCoinsCacheEntry::DIRTY) {
if (it->second.Flags() & CCoinsCacheEntry::DIRTY) {
// Same optimization used in CCoinsViewDB is to only write dirty entries.
map_[it->first] = it->second.coin;
if (it->second.coin.IsSpent() && InsecureRandRange(3) == 0) {
map_[it->first] = it->second.GetCoin();
if (it->second.GetCoin().IsSpent() && InsecureRandRange(3) == 0) {
// Randomly delete empty entries on write.
map_.erase(it->first);
}
Expand All @@ -84,7 +89,7 @@ class CCoinsViewCacheTest : public CCoinsViewCache
size_t ret = memusage::DynamicUsage(cacheCoins);
size_t count = 0;
for (const auto& entry : cacheCoins) {
ret += entry.second.coin.DynamicMemoryUsage();
ret += entry.second.GetCoin().DynamicMemoryUsage();
++count;
}
BOOST_CHECK_EQUAL(GetCacheSize(), count);
Expand Down Expand Up @@ -583,11 +588,14 @@ static size_t InsertCoinsMapEntry(CCoinsMap& map, CAmount value, char flags)
}
assert(flags != NO_ENTRY);
CCoinsCacheEntry entry;
entry.flags = flags;
SetCoinsValue(value, entry.coin);
entry.Flags(flags);
entry.MutableCoin([&](Coin& c) {
SetCoinsValue(value, c);
});

auto inserted = map.emplace(OUTPOINT, std::move(entry));
assert(inserted.second);
return inserted.first->second.coin.DynamicMemoryUsage();
return inserted.first->second.GetCoin().DynamicMemoryUsage();
}

void GetCoinsMapEntry(const CCoinsMap& map, CAmount& value, char& flags)
Expand All @@ -597,12 +605,12 @@ void GetCoinsMapEntry(const CCoinsMap& map, CAmount& value, char& flags)
value = ABSENT;
flags = NO_ENTRY;
} else {
if (it->second.coin.IsSpent()) {
if (it->second.GetCoin().IsSpent()) {
value = SPENT;
} else {
value = it->second.coin.out.nValue;
value = it->second.GetCoin().out.nValue;
}
flags = it->second.flags;
flags = it->second.Flags();
assert(flags != NO_ENTRY);
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/test/fuzz/coins_view.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,15 +116,15 @@ FUZZ_TARGET_INIT(coins_view, initialize_coins_view)
CCoinsMap coins_map;
while (fuzzed_data_provider.ConsumeBool()) {
CCoinsCacheEntry coins_cache_entry;
coins_cache_entry.flags = fuzzed_data_provider.ConsumeIntegral<unsigned char>();
coins_cache_entry.Flags(fuzzed_data_provider.ConsumeIntegral<unsigned char>());
if (fuzzed_data_provider.ConsumeBool()) {
coins_cache_entry.coin = random_coin;
coins_cache_entry.MutableCoin([&](Coin& c) { c = random_coin; });
} else {
const std::optional<Coin> opt_coin = ConsumeDeserializable<Coin>(fuzzed_data_provider);
if (!opt_coin) {
return;
}
coins_cache_entry.coin = *opt_coin;
coins_cache_entry.MutableCoin([&](Coin& c) { c = *opt_coin; });
}
coins_map.emplace(random_out_point, std::move(coins_cache_entry));
}
Expand Down
6 changes: 3 additions & 3 deletions src/txdb.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,12 +106,12 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) {
batch.Write(DB_HEAD_BLOCKS, Vector(hashBlock, old_tip));

for (CCoinsMap::iterator it = mapCoins.begin(); it != mapCoins.end();) {
if (it->second.flags & CCoinsCacheEntry::DIRTY) {
if (it->second.Flags() & CCoinsCacheEntry::DIRTY) {
CoinEntry entry(&it->first);
if (it->second.coin.IsSpent())
if (it->second.GetCoin().IsSpent())
batch.Erase(entry);
else
batch.Write(entry, it->second.coin);
batch.Write(entry, it->second.GetCoin());
changed++;
}
count++;
Expand Down

0 comments on commit a6dc21e

Please sign in to comment.