Merge GetCoins and HaveCoins into CCoinsViewCache::AccessCoins #4822

pull sipa wants to merge 1 commits into bitcoin:master from sipa:bettercache3 changing 7 files +54 −51

sipa commented at 7:34 PM on September 2, 2014: member

Both GetCoins and HaveCoins require a hashtable lookup, while most often we need both. Combine them into AccessCoins which returns a pointer rather than a reference (and gets rid of the overloaded name).

Also replace some calls to the (copying) GetCoins with AccessCoins.
sipa renamed this:
~~Combine CCoinsViewCache's HaveCoins and const GetCoins into AccessCoins~~
Merge GetCoins and HaveCoins into CCoinsViewCache::AccessCoins
on Sep 2, 2014
sipa force-pushed on Sep 2, 2014

in src/coins.cpp:None in 494f003d44 outdated

 165 | @@ -162,9 +166,9 @@ unsigned int CCoinsViewCache::GetCacheSize() const {
 166 |  
 167 |  const CTxOut &CCoinsViewCache::GetOutputFor(const CTxIn& input) const
 168 |  {
 169 | -    const CCoins &coins = GetCoins(input.prevout.hash);
 170 | -    assert(coins.IsAvailable(input.prevout.n));
 171 | -    return coins.vout[input.prevout.n];
 172 | +    const CCoins* coins = AccessCoins(input.prevout.hash);

laanwj commented at 6:25 AM on September 3, 2014:

Now needs a nullpointer check (IIRC null-ness was previously catched using an assert() ).

sipa commented at 7:08 AM on September 3, 2014:

Done (and in several other places).

laanwj commented at 6:30 AM on September 3, 2014: member

utACK, makes sense to distinguish "access to read" and "access to modify"

in src/coins.cpp:None in 494f003d44 outdated

 203 | @@ -207,10 +204,10 @@ double CCoinsViewCache::GetPriority(const CTransaction &tx, int nHeight) const
 204 |      double dResult = 0.0;
 205 |      BOOST_FOREACH(const CTxIn& txin, tx.vin)
 206 |      {
 207 | -        const CCoins &coins = GetCoins(txin.prevout.hash);
 208 | -        if (!coins.IsAvailable(txin.prevout.n)) continue;
 209 | -        if (coins.nHeight < nHeight) {
 210 | -            dResult += coins.vout[txin.prevout.n].nValue * (nHeight-coins.nHeight);
 211 | +        const CCoins* coins = AccessCoins(txin.prevout.hash);
 212 | +        if (!coins->IsAvailable(txin.prevout.n)) continue;

laanwj commented at 6:31 AM on September 3, 2014:

Same here (many more below, won't comment them all)

sipa force-pushed on Sep 3, 2014
sipa force-pushed on Sep 3, 2014

Combine CCoinsViewCache's HaveCoins and const GetCoins into AccessCoins.

The efficient version of CCoinsViewCache::GetCoins only works for known-to-exist
cache entries, requiring a separate HaveCoins call beforehand. This is
inefficient as both perform a hashtable lookup.

Replace the non-mutable GetCoins with AccessCoins, which returns a potentially-NULL
pointer. This also decreases the overloading of GetCoins.

Also replace some copying (inefficient) GetCoins calls with equivalent AccessCoins,
decreasing the copying.

629d75faac

sipa force-pushed on Sep 3, 2014
BitcoinPullTester commented at 10:34 AM on September 4, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4822_629d75faac84bc0a00533d01dd291a4e6394a51f/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
TheBlueMatt commented at 5:23 AM on September 9, 2014: member

utACK
laanwj commented at 3:14 PM on September 9, 2014: member

Tested ACK
sipa merged this on Sep 9, 2014
sipa closed this on Sep 9, 2014
sipa referenced this in commit f7cdcb80ab on Sep 9, 2014
MarcoFalke locked this on Sep 8, 2021

Contributors

Linked (view graph)

#4834 Simplify cache interface and drop fully spent unwritten entries