Cache CBlockIndex::GetMedianTimePast #7650

pull promag wants to merge 1 commits into bitcoin:master from uphold:enhancement/cache-getmediantimepast changing 1 files +17 −8

promag commented at 10:37 AM on March 8, 2016: member

This change prevents calculating the block median time past more than once.
Cache CBlockIndex::GetMedianTimePast 1827430d59
morcos commented at 2:48 PM on March 8, 2016: member

Can you provide some data on any benefit brought by this change? Is GetMedianTimePast called often enough to warrant this?
promag commented at 3:17 PM on March 8, 2016: member

@morcos actually no, just saw something that could be computed once. Maybe later I'll check it.
sipa commented at 8:21 PM on March 8, 2016: member

It adds a few megabytes of extra RAM usage. That's not much, but I'd like to see a benefit first.

in src/chain.h:None in 1827430d59

 250 | +          const CBlockIndex* pindex = this;
 251 | +          for (int i = 0; i < nMedianTimeSpan && pindex; i++, pindex = pindex->pprev)
 252 | +              *(--pbegin) = pindex->GetBlockTime();
 253 | +
 254 | +          std::sort(pbegin, pend);
 255 | +          nMedianTimePast = pbegin[(pend - pbegin)/2];

sipa commented at 8:42 PM on March 9, 2016:

Assignment to const variable.

promag commented at 8:48 PM on March 9, 2016:

Yes I know. I'm still wondering if I'm going to improve this.

laanwj commented at 9:22 AM on March 11, 2016: member

It adds a few megabytes of extra RAM usage. That's not much, but I'd like to see a benefit first.

Agree. Let's not make the CBlockIndex structure bigger without thorough motivation. This structure is kept around in memory for every existing block since the genesis block.
jtimon commented at 5:17 PM on March 16, 2016: contributor

For what is worth, in https://github.com/jtimon/bitcoin/commits/jt (needs rebase on top of a backport of #7575, once it is merged) I'm considering that some callers may want to reimplement the method/function for caching the mediantime somehow (maybe some callers only cache the median time of the tip, I don't know). So, the way it is written, I don't see is as very disruptive to my libconsensus refactor plans (assuming anybody else apart from me cares about that branch at all), even if it looks incompatible with tiny steps I have proposed in the past like #6009.

Having said that, I don't have a strong opinion for bitcoin core one way or another. But some benchmarking to see if the extra memory is justifiable would certainly be useful to form an opinion.
laanwj commented at 11:21 AM on March 24, 2016: member

Closing for now. Let me know when you have investigated the performance versus memory compromise made here.
laanwj closed this on Mar 24, 2016
MarcoFalke locked this on Sep 8, 2021

Contributors