Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API change SequenceReader.ctor pass ReadOnlySequence via in #35679

Closed
benaadams opened this Issue Mar 1, 2019 · 14 comments

Comments

Projects
None yet
@benaadams
Copy link
Collaborator

benaadams commented Mar 1, 2019

 public ref partial struct SequenceReader<T> where T : unmanaged, System.IEquatable<T>
 {
-    public SequenceReader(System.Buffers.ReadOnlySequence<T> sequence);
+    public SequenceReader(in System.Buffers.ReadOnlySequence<T> sequence);
 }

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], xmm0

Its 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.

@stephentoub raised the following concern #35678 (comment)

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.

/cc @stephentoub @jkotas @AndyAyersMS

@mikedn

This comment has been minimized.

Copy link
Collaborator

mikedn commented Mar 1, 2019

It seems that the SequenceReader already has AggressiveInlining and if it inlines, the fact that the parameter is passed by value or ref shouldn't matter. If it matters then it's likely because of something that the JIT doesn't do today but could probably do later.

In general I advise caution about passing parameters by reference. If the method doesn't end up being inlined then you might just make things worse. You save a copy at the expense of potentially making a local variable address exposed and killing pretty much optimizations around it. And handling address exposed variables in the JIT is something more difficult to do. The readonly part of in may help a bit with that but then I don't know if the JIT should actually rely on that because it cannot be enforced, much like C++'s const.

@benaadams

This comment has been minimized.

Copy link
Collaborator Author

benaadams commented Mar 1, 2019

Can always add both overloads?

 public ref partial struct SequenceReader<T> where T : unmanaged, System.IEquatable<T>
 {
    public SequenceReader(ReadOnlySequence<T> sequence) : this(in sequence);
    public SequenceReader(in ReadOnlySequence<T> sequence);
 }
@terrajobst

This comment has been minimized.

Copy link
Member

terrajobst commented Mar 19, 2019

@jkotas what's our architectural stance on this? We could offer both overloads and let developers decide; it's an advanced API. If we offer both, the default will be pass by value, with advanced folks being able to force in via call side modifier.

@Wraith2

This comment has been minimized.

Copy link
Collaborator

Wraith2 commented Mar 19, 2019

You can't overload on the basis of in/out/ref parameters iirc. The others could be a static factory method perhaps?

@NickCraver

This comment has been minimized.

Copy link
Member

NickCraver commented Mar 19, 2019

It turns out you can overload ref/out/in and none of them, but not between them - just learned this as well.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Mar 19, 2019

what's our architectural stance on this?

The proposed API is working around deficiency in JIT optimizations. The general stance is that we do not want to be tweaking the public API surface for point-in-time deficiencies in JIT optimizations. Doing so would lead to nonsensical API surface over time. @mikedn 's comment above goes into more details on this: #35679 (comment)

We can make exceptions to this rule if the perf loss is too big to be left on the table, and the JIT fix is too hard. It does not seem to be the case here.

@tannergooding

This comment has been minimized.

Copy link
Member

tannergooding commented Mar 19, 2019

@jkotas, what about the case when an API is not AggressiveInlining. It seems like the JIT should also be able to rationalize an in parameter that is then inlined and do the efficient thing.

ReadOnlySequence<T> is large enough that all currently supported ABIs will cause an implicit shadow copy and pass by reference in the case that the method isn't inlined, so it would normally be the efficient thing to pass these parameters by in to avoid the shadow copy.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Mar 19, 2019

what about the case when an API is not AggressiveInlining

It is not the case with this proposal - I expect that the proposed API would be marked with AggressiveInlining as well.

@ghuntley

This comment has been minimized.

Copy link
Member

ghuntley commented Mar 19, 2019

@terrajobst

This comment has been minimized.

Copy link
Member

terrajobst commented Mar 19, 2019

@jkotas

So that means we shouldn't make it in then and improve the JIT. Presumably, if the loss is too high, would you be opposed to doing the overload?

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Mar 19, 2019

if the loss is too high, would you be opposed to doing the overload?

Correct. As I have said above, I do not think it is the case here.

@terrajobst

This comment has been minimized.

Copy link
Member

terrajobst commented Mar 19, 2019

Sounds good, so I'm closing.

@terrajobst terrajobst closed this Mar 19, 2019

@benaadams

This comment has been minimized.

Copy link
Collaborator Author

benaadams commented Mar 19, 2019

The proposed API is working around deficiency in JIT optimizations.

Presumably the Jit not being able to drop refs when inlining is also a deficiency in JIT optimization?

@AndyAyersMS

This comment has been minimized.

Copy link
Member

AndyAyersMS commented Mar 19, 2019

Presumably the Jit not being able to drop refs when inlining is also a deficiency in JIT optimization?

Yes. The jit can often fix this, but not always, even when it arguably should.

And some cases can't ever be fixed, as @mikedn notes. Say the ref propagates to a method that does not get inlined -- in cases like that all accesses to the struct can be pessimized, even if that call is rare.

With "by value" semantics the jit has some natural points at where it can safely change struct representation for best optimization. With "by ref" the jit needs to be able to see and possibly modify all touches to do this, or allow for switching representations of the same struct variable over regions of code. Going forward we're more likely to invest in by-value opts (eg struct copy prop & reverse copy prop) than in some kind of region-based promotion to make the messy "by ref" cases more tractable. Admittedly we're not great at by-value opts today, but we are slowly chipping away at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.