Aspnetcore: Use "in" modifier with large readonly structs

[JamesNK]

re: https://github.com/aspnet/AspNetCore/pull/7169#issuecomment-459535498

There are performance benefits with using the in modifier to avoid copying large readonly structs.

Note that adding in to a public method is a breaking change. Only internal methods should be updated.

https://docs.microsoft.com/en-us/dotnet/csharp/write-safe-efficient-code#apply-the-in-modifier-to-readonly-struct-parameters-larger-than-systemintptrsize

[JamesNK]

@benaadams Hey is this something you probably have experience with. Is there an article, guide or tools you recommend for helping identify what structs should use in

[benaadams]

Caution putting it in a public api as its a breaking change to remove it and it could prevent future Jit optimizations from working https://github.com/dotnet/corefx/issues/35679#issuecomment-468562023.

The main downside is it locks the struct into being memory accessed rather than being decomposed into registers in the method where it is used; as its via ref so it must operate on the reference; whereas by val it is disconnected from the original copy. It also forces the struct in the caller to be a memory location rather than registers (though to pass by val it then has to construct a copy on the stack which it then passes a reference to, so may be much of a muchness)

An example would be ReadOnlySpan; you wouldn't want to pass that via in as that would force each access of an element to look up from memory what the span is pointing to and then add the offset; whereas by val the Jit would load what its pointing to into a register; only accessing memory once, and then use offsets on that register.

You can manually read a property from a reference passed to a local; which would achieve the same effect as what the Jit would try to do with a by val passed struct, but then its manual (Aside: you wouldn't want to manually decompose a span this way as the Jit has other optimizations that apply to it that you'd loose out on)

If its pointer sized or less definitely don't pass via in; as they get passed via register anyway so its a deoptimization to pass a reference when you could just pass the thing.

If you are doing single reads of properties then in is probably helpful; its when you do multiple reads either directly (which you can resolve manually above by reading once to local); or via inlined methods. If its a non-inlined method then its again likely forced to stack as it needs to operate on a memory location (as the callee can't operate on the registers of the caller)

tl;dr

• never at <= 8bytes, <= 1x pointer; as that only has downsides
• sometimes value at >8 bytes, >1 x pointer; if methods are called on it and those methods do not inline, or the method immediately takes its own copy (e.g. a constructor)
• otherwise the struct should be large (>16 bytes, 2 x pointer);
• if you do pass via in you miss out on potential register optimizations; so if you read a property multiple times its worth manually reading that property to a local then using the local.
• caution in public apis, due to breaking changes (adding an additional in overload is not breaking, but adding a non-in overload when there is one already is breaking as that become the preferred overload)

Also worth measuring the effects, or something like that

YMMV :)

[rynowak]

https://blogs.msdn.microsoft.com/seteplia/2018/05/03/avoiding-struct-and-readonly-reference-performance-pitfalls-with-errorprone-net/

[Eilon]

We clearly didn't get to this in preview4, and it's not going to happen in preview5 either.

At this point do we close it, or is it worthy as a Backlog item for future consideration?

[JamesNK]

It's worth doing, there is no urgency. Backlogging.

[anurse]

I'm not sure this is terribly helpful without a specific set of things or an area to look in. Otherwise it just sits in our dispatch queue waiting for someone to assign an area label. I actually propose closing this until we have specific structs that are causing problems. Alternatively we could apply the area-infrastructure label. Thoughts @JamesNK @davidfowl @Pilchie ?

[benaadams]

until we have specific structs that are causing problems.

ReadOnlySequence is an obvious one https://github.com/aspnet/AspNetCore/pull/11052

[anurse]

Yeah, my concern is just that a vague issue doesn't help us a lot here. Making structs use in is a _solution_ not a _problem_. If there are performance problems that can be solved by making more structs use in, then we can track and fix those. Otherwise this issue is probably just going to get punted around because it's value is unknown.

[JamesNK]

👍