NullTxOut comparer

[MithrilMan]

working on DBreezeCoinView performance enhancment, i stepped into this line https://github.com/stratisproject/StratisBitcoinFullNode/blob/22abd08c3dfc624f0c60c06cba887da78b43ce05/src/Stratis.Bitcoin.Features.Consensus/CoinViews/DBreezeCoinView.cs#L224

coin.ToCoins() implementation is this https://github.com/stratisproject/StratisBitcoinFullNode/blob/19be1dbb127dd3727345e73da1eb41351428bda3/src/Stratis.Bitcoin/Utilities/UnspentOutputs.cs#L136-L153

I'd like to point you to coins.Outputs.Add(output == null ? Coins.NullTxOut : output);

here if the output is null, we set it with a Coin special case instance Coins.NullTxOut

next line coins.ClearUnspendable();, internally do this https://github.com/stratisproject/StratisBitcoinFullNode/blob/22abd08c3dfc624f0c60c06cba887da78b43ce05/src/NBitcoin/BitcoinCore/Coins.cs#L278-L288

so it sets a txOut to Coins.NullTxOut if o.ScriptPubKey.IsUnspendable so here we are fine, we have a list of output that may be set to a logic null value rapresented by Coins.NullTxOut

The problem lies in the last line this.Cleanup(); that's implemented this way:

https://github.com/stratisproject/StratisBitcoinFullNode/blob/22abd08c3dfc624f0c60c06cba887da78b43ce05/src/NBitcoin/BitcoinCore/Coins.cs#L103-L115

where this.IsNull is private bool IsNull(TxOut o) => o.Value.Satoshi == -1

so instead of checking only compare references o == Coins.NullTxOut it goes deeper comparing the nested Value.Satoshi value.

I created a test to see performance improvement doing this (I mimicked the class hierarchy to simulate it as close as possible)

https://dotnetfiddle.net/Cm8wc8

checking straight the reference is at least 2x faster on tests I did

Of course we have to be sure that logic null value on outtx are always sent as NullTxOut rather than new TxOut(new Money(-1)...

I havent investigated further but there could be other scenarios like this

[zeptin]

Nice analysis, but is the performance gain non-negligible compared to the amount of time the DBreeze insert takes?

[MithrilMan]

sure it is negligible, nonetheless I don't know how many part of the code are relying on null checks of that kind. About DBreeze insert issue I creaded another issue (with a fix) here https://github.com/stratisproject/StratisBitcoinFullNode/issues/2483