Optimization improvements #15160

Mr-Leshiy commented at 9:45 AM on January 14, 2019: contributor

These changes provide some improvements that potentially can to speed up the work of bitcoind. Cause i dont have enough resources i haven't done any tests, so i dont have any results which can approve that these changes can increase the speed of work.

SetTip method The main idea of this change is if we want to add a new block to our vector and this block can to join to the active chain, we just add it and dont rebuild the whole vector.

GetAncestor method If the previous block contains in our active chain in the vector, we can obtain the needed block from the vector and dont have to walk throught the references.

Some optimisation with the adding a new tip to the blockchain vector and with the finding a block ancestor 8ec1aa528f

in src/chain.cpp:100 in 8ec1aa528f outdated

  96 | @@ -89,6 +97,9 @@ const CBlockIndex* CBlockIndex::GetAncestor(int height) const
  97 |      const CBlockIndex* pindexWalk = this;
  98 |      int heightWalk = nHeight;
  99 |      while (heightWalk > height) {
 100 | +        if (chainActive.Contains(pindexWalk->pprev))

promag commented at 11:44 AM on January 14, 2019:

Nack, shouldn't access chainActive.

Mr-Leshiy commented at 3:32 PM on January 14, 2019:

Why not ? It is a global variable that contains the information about active chain, why we cant use it in these place. Or there are some another reasons to not use this variable?

promag commented at 3:37 PM on January 14, 2019:

But this is not about the active chain, this is about the ancestor of the given block index. The goal is not to get a block at a given height in the active chain. Also, keep in mind that the active chain is shared across multiple threads.

Mr-Leshiy commented at 3:48 PM on January 14, 2019:

Of course, but if the previous block contains in the active chain an ancestor of the current block will contains in the active chain too. About the multiple threads, i just read the information from the variable in this case i dont think that will be some problems with it.

promag commented at 3:59 PM on January 14, 2019:

It needs to lock cs_main which in some places we want to avoid.

Anyway, this "optimization" should be moved to the call sites, if any.

Mr-Leshiy commented at 6:54 PM on January 14, 2019:

You mean that it would be mor preferable to remove it from the GetAncestor method and replace at all places where that method calls ?

promag commented at 11:01 PM on January 14, 2019:

if i'm not mistaken, if at the call site cs_main is locked then GetAncestor could be replaced by CChain::operator[].

Mr-Leshiy commented at 9:14 AM on January 15, 2019:

If the previous of the current block contains in the active chain, yes we can replace.

laanwj added the label Validation on Jan 14, 2019

promag commented at 11:02 PM on January 14, 2019: member

Note travis error "A new circular dependency in the form of "chain -> validation -> chain" appears to have been introduced."

in src/chain.cpp:19 in 8ec1aa528f outdated

  14 | @@ -14,9 +15,16 @@ void CChain::SetTip(CBlockIndex *pindex) {
  15 |          return;
  16 |      }
  17 |      vChain.resize(pindex->nHeight + 1);
  18 | -    while (pindex && vChain[pindex->nHeight] != pindex) {
  19 | +
  20 | +	// compare with the current vector tip after resizing

practicalswift commented at 10:23 AM on January 15, 2019:

Don't use tabs. Applies throughout this PR :-)

in src/chain.cpp:22 in 8ec1aa528f outdated

  19 | +
  20 | +	// compare with the current vector tip after resizing
  21 | +	if (vChain.size() >= 2 && pindex->pprev == vChain[vChain.size() - 2]) {
  22 |          vChain[pindex->nHeight] = pindex;
  23 | -        pindex = pindex->pprev;
  24 | +	}

practicalswift commented at 10:23 AM on January 15, 2019:

Line ends with whitespace. Please remove :-)

Some fixes e3188c96d1

Delete tab 6024afea60

in src/chain.cpp:23 in 8ec1aa528f outdated

  20 | +	// compare with the current vector tip after resizing
  21 | +	if (vChain.size() >= 2 && pindex->pprev == vChain[vChain.size() - 2]) {
  22 |          vChain[pindex->nHeight] = pindex;
  23 | -        pindex = pindex->pprev;
  24 | +	} 
  25 | +	else {

practicalswift commented at 10:24 AM on January 15, 2019:

If an else has a brace on one side it should have it on both sides :-)

Mr-Leshiy commented at 12:07 PM on January 15, 2019: contributor

I have fixed these places. @practicalswift, thanks for review. And what about the GetAncestor method. As i understand i cant put locks there and without that we have unsafe code. So it will be preferable to remove these lines ? if (chainActive.Contains(pindexWalk->pprev)) return chainActive[height]; But i think that to get the ancestor from the active chain if we can is sensible idea.

sdaftuar commented at 1:39 PM on January 15, 2019: member

NACK

SetTip method The main idea of this change is if we want to add a new block to our vector and this block can to join to the active chain, we just add it and dont rebuild the whole vector.

The existing code already does this without rebuilding the whole vector -- after the tip is added, the loop will exit.

Also @promag is correct that GetAncestor() should not be accessing chainActive.

Some advice for optimization PRs:

If you're not sure what the performance change of small changes like this might be, you can write a microbenchmark, see src/bench. In this case, I imagine that would show no improvement in the SetTip() performance.
In general please keep in mind that contributors have been optimizing the code for years, so it's helpful to take a comprehensive look at what's happening as a whole before deciding to try optimizing some small part. Often we get contributions that would optimize a function that is rarely invoked, or not part of any critical path, which typically aren't worth merging if they make the code more complex.

Mr-Leshiy closed this on Jan 15, 2019

Mr-Leshiy reopened this on Jan 15, 2019

Mr-Leshiy commented at 2:29 PM on January 15, 2019: contributor

@sdaftuar, thank you for your comment. Maybe i will close this pull request. Thank you everyone for discussion i will keep in mind all remarks in future work.

Mr-Leshiy closed this on Jan 15, 2019

DrahtBot locked this on Dec 16, 2021