fees: optimize decay #11194
fees: optimize decay #11194
Conversation
Rather than jumping around, visit elements in-order. This produces a noticeable speedup.
An optimization like this would be important for IBD. |
|
@gmaxwell Indeed. When profiling IBD for the first few hundred thousand blocks, some of my results showed this function alone accounting for ~5-10% of all cpu time spent on the message handling thread. This change reduces those percentages to roughly half iirc, but obviously avoiding it entirely when possible would be best. |
How about not Edit: when in IBD. |
|
FWIW, I benchmarked a sync completely bypassing CBlockPolicyEstimator::processBlock and it was effectively the same speed (0.5% faster, but that could well be noise). |
|
You mean speed in IBD, not CPU usage right? |
|
Yes, though on this host (and in this benchmark) IBD is more or less cpu limited. I didn't intend to suggest that we shouldn't optimize here, but it's apparently not a huge it in IBD as cfields was initially concerned about it being. |
| } | ||
| assert(txCtAvg.size() == buckets.size()); | ||
| for (auto& bucket : txCtAvg) { | ||
| bucket *= decay; |
There was a problem hiding this comment.
Would it might be more clear to write the following
auto applyDecay = [&decay, &buckets](std::vector<double>& transactions)
{
assert(transactions.size() == buckets.size());
for (double& bucket : transactions)
{
bucket *= decay;
}
};
for (auto& elem : confAvg)
{
applyDecay(elem);
}
for (auto& elem : failAvg)
{
applyDecay(elem);
}
applyDecay(avg);
applyDecay(txCtAvg);
There was a problem hiding this comment.
Please lets not add more asserts in non-consensus/net code. Without the asserts happy with this or @Dllieu's suggestion.
| The last travis run for this pull request was 324 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened. |
|
Last activity on this was in 2017, going to close this and add "up for grabs". Let me know if you want me to reopen it. |
As discussed briefly last week with @morcos and @sdaftuar.
Rather than jumping around, visit elements in-order. This produces a noticeable speedup.
I added the assertions because it's not entirely clear from the existing code if the bucket size is allowed to differ from the individual sizes. I can't imagine that that could be the case, so I'll remove them if preferred.
Additionally, I assume we could skip UpdateMovingAverages() altogether if no entries have ever been added, but it's not obvious to me how to do that without breaking something.