I'm not sure this is the right fix. Changing in to ref should only affect code gen in cases where the the C# compiler had to generate a silent copy in order to avoid potential mutation when it promised there wouldn't be. For example, it doesn't know that DbRow.Location won't actually mutate this, and thus when the callee accesses Location on a struct passed by ref via in, it needs to make a defensive copy of the struct, just in case Location mutated this. Seems the right fix then would be to annotate Location (and any other such properties) as readonly, in which case the compiler will know it can skip the defensive copy.

(That said, if you want to change these from in to ref for other reasons, or in addition to making the relevant members readonly, that's fine.)

Seems the right fix then would be to annotate Location (and any other such properties) as readonly, in which case the compiler will know it can skip the defensive copy.

Yes we do that in other places -- at least one other in usage was not changed here because of that.

I'll look at changing the existing code and report back.

However using ref seems like it is less error-prone than having to verify every member access has readonly applied.

I'll look at changing the existing code and report back.

I was able to change the DbRow to use in+readonly but not the serializer stack based struct. That is mutable and can't be made readonly.

That is mutable and can't be made readonly.

What methods are you calling that can't be made readonly? That suggests they actually mutate this, in which case passing with ref may result in different problems

To be clear, I'm fine in general with a change to ref. But keep in mind that there are potential problems with it, too: the reason you get the defensive copy with in is because the target may mutate... if that protection was necessary and you change to ref, now you may have a functional problem.

What methods are you calling that can't be made readonly?

As mentioned it is currently error-prone to determine if a given method should use in w.r.t performance, not mutability. Even though the method doesn't mutate, the struct may not be adorned with readonly appropriately. I could go through and annotate the struct(s) in question (ReadStackFrame and possibily WriteStackFrame) with readonly but that is not worth it at this point IMO since there is only one method that would affect performance (which was changed in the PR from in to ref).

Since this in internal code, there is not a concern about accidently mutating state. Originally when the code was written I was not aware of the performance side-affect of in.

It is unfortunate IMO that the defensive copy is done or needs to be done - a different approach would be to not allow at compile time calling a non-readonly field\property when in is used.

Since this in internal code, there is not a concern about accidently mutating state.

Why not?

If you use in, we may accidentally get silent defensive copies if members aren't properly annotated as readonly, resulting in potential perf degradation.

If you use ref, we may accidentally get silent mutation, resulting in potential functional degradation.

a different approach would be to not allow at compile time calling a non-readonly field\property when in is used

This can be achieved with an analyzer. Someone just needs to write one. Maybe @jaredpar's team already has one or has one in the works?

@stephentoub

This can be achieved with an analyzer. Someone just needs to write one. Maybe @jaredpar's team already has one or has one in the works?

The one that we've been looking at using in our own code base is ErrorProne.NET: https://github.com/SergeyTeplyakov/ErrorProne.NET

Since this in internal code, there is not a concern about accidently mutating state.

Why not?
If you use in, we may accidentally get silent defensive copies if members aren't properly annotated as readonly, resulting in potential perf degradation.
If you use ref, we may accidentally get silent mutation, resulting in potential functional degradation.

Perhaps we should modify every internal (and public?) struct to adorn with readonly modifers so consumers can properly use in. For this particular case, one method, I didn't think it was worth the cost:benefit.

Once such as analyzer usage is pervasive (built-in or prescriptive in some way) then I think it is feasible to use in on mutable structs (that can't be adorned with readonly at the type level). Until then it is hit-and-miss whether any code changes will cause an perf issue. Currently to verify I have to use the disassembler :(

Currently to verify I have to use the disassembler

An analyzer like ErrorProne.NET will tag such places and let you know when a defensive copy is being made. In general, I think using in is better as it clearly indicates that you are not expecting a mutation. We then can mark either structs or methods as readonly to help guarantee that contract and avoid implicit copies.

Hopefully we can get warning waves eventually and it can just be a compiler warning instead.

In general, I think using in is better as it clearly indicates that you are not expecting a mutation.

But if a mutation does happen, it is silent, which is a likely bug.

I added my thoughts at https://github.com/dotnet/corefx/issues/40166

/azp run corefx-ci

Committing this; there is likely future work to improve in semantics here and elsewhere once misuses with in can be detected (perf hit when missing readonly modifiers or accidental mutability which are lost\not propagated up the stack).

@tannergooding mentioned https://github.com/SergeyTeplyakov/ErrorProne.NET/blob/master/ReadMe.md..@stephentoub did you try that one?

I tried it, but was not able to install the vsix in VS 2019 (and referencing the NuGet package didn't work within corefx libraries and I don't know why). Using the NuGet package in a standalone .NET Core 3.0 app worked as expected though.

8/8/2019 3:14:23 PM - Install Error : Microsoft.VisualStudio.ExtensionManager.MissingReferencesException: This extension cannot be installed because the following references are missing:
-Microsoft.VisualStudio.Component.CoreEditor (Microsoft.VisualStudio.Component.CoreEditor)
-Microsoft.VisualStudio.Component.Roslyn.LanguageServices (Microsoft.VisualStudio.Component.Roslyn.LanguageServices)

I have reached out to @SergeyTeplyakov to help fix the VSIX issue.