[No Merge][WIP] SequenceReader pass ReadOnlySequence via in#35678
[No Merge][WIP] SequenceReader pass ReadOnlySequence via in#35678benaadams wants to merge 1 commit intodotnet:masterfrom
Conversation
| public ref partial struct SequenceReader<T> where T : unmanaged, System.IEquatable<T> | ||
| { | ||
| public SequenceReader(System.Buffers.ReadOnlySequence<T> sequence) { throw null; } | ||
| public SequenceReader(in System.Buffers.ReadOnlySequence<T> sequence) { throw null; } |
There was a problem hiding this comment.
Aren't all of these breaking changes?
There was a problem hiding this comment.
(And if they're not because we haven't shipped any of these yet, it's still a public API change that needs to go through a public API review process.)
There was a problem hiding this comment.
Should be able to change SequenceReader as its only 3.0?
There was a problem hiding this comment.
Should be able to change SequenceReader as its only 3.0?
If you want to change it, please open an issue for it as it needs to go through API review.
I would, however, urge us to avoid polluting methods like this with in unless we find that a) it makes a meaningful difference and b) we expect that difference will never be able to captured by JIT-level changes instead.
There was a problem hiding this comment.
Drops 32-bytes of stack zeroing and this from call site
vmovdqu xmm0, qword ptr [rbx]
vmovdqu qword ptr [rbp-1D0H], xmm0
vmovdqu xmm0, qword ptr [rbx+16]
vmovdqu qword ptr [rbp-1C0H], xmm0Its a 4 element struct so its unlikely to be covered by calling convention; however it is also an Aggressive Inline (albeit a frighteningly large one) and there has been speculation that the Jit could eventually elide the copy (@mikedn mentioned it on the ValueTaskAwaiter PR); though it doesn't currently.
Will open api issue for discussion.
There was a problem hiding this comment.
albeit a frighteningly large one
Yeah, I'm not sure why it's annotated that way. Seems like a "this helped a microbenchmark" kind of usage that could actually have an overall negative impact, but I've not looked in detail.
There was a problem hiding this comment.
I assume as its an even more chunky struct; so if it didn't inline the constructor would be immediately be followed by 2x 104 byte copies
Type layout for 'SequenceReader`1'
Size: 104 bytes. Paddings: 23 bytes (%22 of empty space)
Once for copying from the return stack to method stack; followed by method stack to method stack for the assignment to local (as seen in dotnet/coreclr#22738)
There was a problem hiding this comment.
e.g. this innocuous line
var start = default(SequenceReader<byte>);is
G_M20026_IG23:
33C9 xor rcx, rcx
488D85F8FEFFFF lea rax, bword ptr [rbp-108H]
C5F857C0 vxorps xmm0, xmm0
C5FA7F00 vmovdqu qword ptr [rax], xmm0
C5FA7F4010 vmovdqu qword ptr [rax+16], xmm0
C5FA7F4020 vmovdqu qword ptr [rax+32], xmm0
C5FA7F4030 vmovdqu qword ptr [rax+48], xmm0
C5FA7F4040 vmovdqu qword ptr [rax+64], xmm0
C5FA7F4050 vmovdqu qword ptr [rax+80], xmm0
48894860 mov qword ptr [rax+96], rcx
898DF4FEFFFF mov dword ptr [rbp-10CH], ecxThere was a problem hiding this comment.
Though made PR to resolve that dotnet/aspnetcore#8076
benaadams
changed the title
benaadams
changed the title
|
@benaadams do you have rough timeline in mind for this PR? |
|
@karelz needs api review https://github.com/dotnet/corefx/issues/35679 |
|
@benaadams ah ... wanna stop by at API review tomorrow and help push it through? ;) |
karelz
added
blocked
|
Closing per API review. |
4 participants
Its a 32-byte 4 element
readonly structso it avoids a chunky copy in the call site/cc @JeremyKuhne @ahsonkhan @pakrym