Passing spans by value

[YohDeadfall]

It makes zero sense to pass spans by references if that span isn't replaced by another one (I mean changing the address it points to). The same applies to arrays as well.

[siwatanejo]

if you're saying "it make zero sense to pass spans by value", shouldn't the title of the commit msg actually be "Stop passing spans by value"? Otherwise I don't get it.

[YohDeadfall]

My bad, spans must be passed by value. Fixed the description, thanks!

[knocte]

Maybe passing spans by reference, even if not replaced, makes performance better? This way there's no copy needed to be done.

[Kukks]

Maybe passing spans by reference, even if not replaced, makes performance better? This way there's no copy needed to be done.

Maybe, but a Span is a kind of reference to its underlying data, and is designed to be lightweight to copy. Isn't there a slight overhead when there is a reference because it needs to maintain that link. All in all there's probably negligent overhead difference in either option.

I'd be more worried if the Span was being replaced in the containing method, bad practice, but could still introduce bugs here/ @YohDeadfall You didn't happen to notice any instance of this right?

[YohDeadfall]

Maybe passing spans by reference, even if not replaced, makes performance better? This way there's no copy needed to be done.

Spans are two machine word wide value types, so it can be passed through two registers which is dirty cheap. Passing it by a reference leads to double memory reads instead of a single to which the span points too, and because of that it's allocated on the stack while it can be stored in registers completely.

The same applies to passing an array by a reference.

I'd be more worried if the Span was being replaced in the containing method, bad practice, but could still introduce bugs here/ @YohDeadfall You didn't happen to notice any instance of this right?

Had the same thought, so checked all methods to be sure that it's not a case. Check NBitcoin/BitcoinStream.cs to be sure that everything is fine there.

Ran tests and it looks fine, no failures.

[NicolasDorier]

This code break untested stuff from shitcoin.

While I agree byref span doesn't make sense, byref array are used because the .ReadWrite is responsible to create the array.

[NicolasDorier]

@YohDeadfall sorry if I am slow, don't hesitate to track me on twitter or the btcpayserver chat if I miss it. I'm so underwater with notifications that I turned them all off :p

[YohDeadfall]

While I agree byref span doesn't make sense, byref array are used because the .ReadWrite is responsible to create the array.

I was worried by that too, but according to following code there are now allocations:

https://github.com/MetacoSA/NBitcoin/blob/0b03c32a7cd3474dafc5634ab241cf9bf6134437/NBitcoin/BitcoinStream.cs#L334-L337

https://github.com/MetacoSA/NBitcoin/blob/0b03c32a7cd3474dafc5634ab241cf9bf6134437/NBitcoin/BitcoinStream.cs#L415-L425

https://github.com/MetacoSA/NBitcoin/blob/0b03c32a7cd3474dafc5634ab241cf9bf6134437/NBitcoin/BitcoinStream.cs#L444-L470

Could you point me where exactly I broke code?

[NicolasDorier]

you are right, as said on twitter DM, can you remove from BitcoinStream the methods with ref not making sense? I guess they did made sense in the past, but some refactoring made them irrelevant.

[YohDeadfall]

There's a method for List<T> serialization ad deserialization, but I wouldn't touch it without improving the design of the whole library and bringing nullability. So, for this pull request I have no more changes.