Add skipautoprops feature #912
Conversation
| get { return _output; } | ||
| set { _output = value; } | ||
| } | ||
| public string Output { get; set; } |
There was a problem hiding this comment.
One reason I use explicit backing fields is to prevent the overhead of frequent method calls. It's probably bad for performance
There was a problem hiding this comment.
I think that autoprops are inlined by jitter, take a look here https://sharplab.io/#v2:C4LghgzgtgPgAgJgIwFgBQcDMACR2DC2A3utmbjvgBQCUp5Ja5z2ACgE4D2ADhNgLzYkABgDc9MgF8JFbAEsAdsDZdexAOYBTYKIjbR0tIfTosuBNgCy6Rsxlm4AFmwAVTRGC0ZtlmUIBjAWwFTQB3AlpxJl9cJABOKn8AOg4eCBoo5kk7I3QgA=
Also I agree in case of hot loops, but here this properties are accessed only one time on msbuild task usage.
What do you think?
There was a problem hiding this comment.
@jkotas, sorry to bother you, can you confirm that autoprops are inlined by jitter?
There was a problem hiding this comment.
I was referring more to the general case because we're targeting .NET standard. Inlining of auto props is an implementation detail of the JIT, at least I don't recall it being in the CLI spec. Do we have any plans to support other runtimes beyond .NET Core?
There was a problem hiding this comment.
Do we have any plans to support other runtimes beyond .NET Core?
We support .NET Framework at the moment(not yet on collectors but I'm waiting some answer from vstest team). In this case(inlining apart) I would prefer clean syntax over perf...I mean this code is called only one time for asm, we spend a lot of time on IL instrumentation(I think mangnitude orders), btw I won't oppose if you prefer custom back field, let me know!It was instinctive from my side 😄
There was a problem hiding this comment.
Any runtime where performance matters will inline auto-properties just fine. It is certainly the case for both .NET Framework and .NET Core.
If you wanted to optimize for hypothetical runtimes that are missing basic optimizations, your code would look very unnatural. You would want to e.g. prefer public fields over properties, minimize number of methods and making methods as big as possible, etc. It does not sounds like a good idea.
tonerdo
dismissed
their stale review
Didn't realize requesting changes actually blocks merging. They were mostly just comments to take note of
closes #328
High pri feat as planned https://github.com/coverlet-coverage/coverlet/blob/master/Documentation/Roadmap.md#high-priority
I did also manual tests on my local with all three drivers
cc: @tonerdo @petli