Zip refactoring #639
Zip refactoring #639
Conversation
- introduce CustomZip with separate behavior by source (internal)
- use CustomZip for EquiZip, ZipLongest and ZipShortest
- add ValueTuple overloads
- use t4 file to have up to 8 input sequences.
There was a problem hiding this comment.
This refactoring has its heart in the right place from a code reuse and consolidation view, but unfortunately it's much worse than the current implementation as it will allocate an array (of arguments) on each iteration of the loop; for sequences of a thousand elements, it will produce a thousand arrays of garbage. Even if this is solved somehow, there are other sorts of allocations like configurations, paddings and enumerators that would need to be addressed to have (at the very least) the same runtime characteristics as before.
|
I was thinking of removing the ZipSourceConfiguration part (it doesn't handle all the case anyway) and use (3) differents implementation of a ZipIteration strategy that will receive the (wrapped) iterators only once. The final loop will then look like while(iterationStrategy.MoveNext())
{
yield return resultSelector(firstZipIterator.Current, ...);
}Avoid the wrapping of the enumerator is tough, I don't think they cost so much, am I wrong? The wrapper will:
|
There was a problem hiding this comment.
This is looking much better from the allocation angle. 👍
use t4 file to have up to 8 input sequences.
This is not something that's been agreed on so kindly revert to the previous arity. This can be discussed in a new issue and a separate PR can follow if and when this one is merged. A refactoring PR shouldn't add to the public surface area of the API unless it's a refactoring of the design (but here it's just the implementation).
Sorry but I need clarification here. Do you want me to remove the tt files and keep the generated code ? |
Not at all. I just don't want new overloads as part of this PR because this is a refactoring of internals that shouldn't be expanding the public API surface as part of the same commit; it just belongs in a separate commit. |
Thank you for the clarifications. I will reduce the overload in this PR and open a new one with the full stuff. |
|
Is there a reason for the split in e8925dc? |
Readability of the .g.tt files. |