-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Description
When using the in modifier on structs passed as method arguments, subsequent calls to the struct's members may cause a performance penalty because a defensive copy of the struct is made. This defensive copy is made when a mutation could occur, such as when calling a method, accessing a property, or setting a field.
For a non-mutable struct, to prevent the defensive copy the readonly modifier should be added to the struct itself and any fields and non-auto properties.
For a mutable struct, the readonly modifier should be added to the fields\properties\methods that do not mutate. However it may be safer to use the ref modifier instead of in in these cases because:
- Maintainability: as non-mutating members are added to the struct over time, the
readonlymodifier may not be added to new members. Callers will incur the performance penalty without knowledge. - Silent mutability: a call to a mutating member will silently mutate the struct meaning it will not cause an error and will not be propagated up the stack. This may or may not be desired in the local method, but often will be a bug.
Based on these I assume the following best practices:
- Do not mutate a struct in a method where it was passed with
in. Instead remove theinand let the copy happen automatically up front or use therefmodifier if that is the desired behavior (see next line below). - Pass mutable structs with
refinstead ofin. If the struct was properlyreadonly-adorned thenincould be used reliably given verification checks (analyzers or compiler warnings). - All structs should be adorned with
readonlyas much as possible so consumers can reliably use theinmodifier without concern for perf due to the defensive copy. Note that theinmodifier was recently added (C# 7.2) so previously existing structs are not likely adorned.
In summary, the in modifier will be commonly misused because it is not obvious that a defensive copy is done, or even allowed to occur. In addition, any members called that do mutate the struct are "silent" which will often be a bug. In order to properly use in one must:
- Have knowledge of the defensive copy and "silent" mutations.
- Have knowledge of whether every member called on a struct has
readonlyapplied or install an analyzer or use a disassembler to be sure the performance penalty or accidental\silent mutation doesn't occur.
Thoughts to address this so in can be reliably used on both mutable and immutable structs:
- Run an analyzer as part of an outer-loop to detect various likely misuses; we may need a way to allow exceptions (e.g. "white list" or special attribute).
- Run an analyzer automatically for all developers (at least for the happy path when using Visual Studio)
- Treat as a compiler warning?
(for lack of a well-known home for this issue, it was created in corefx with area-infrastructure).