IOperationsWrapperSonar: Use compiled expressions

[martin-strecker-sonarsource]

The IOperationWrapperSonar does not follow the pattern used for the other wrappers. This PR rewrites the wrapper to

• Use compiled expressions instead of cached reflection
• Use a readonly struct instead of a class

Before

After

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[andrei-epure-sonarsource]

Can we replace our IOperationWrapper with what StyleCop users, for simplicity?

https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/StyleCop.Analyzers/StyleCop.Analyzers.CodeGeneration/OperationLightupGenerator.cs#L635

[andrei-epure-sonarsource]

@martin-strecker-sonarsource can we use https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/StyleCop.Analyzers/StyleCop.Analyzers.CodeGeneration/OperationLightupGenerator.cs#L21 instead?

see DotNetAnalyzers/StyleCopAnalyzers@9b5db19

[martin-strecker-sonarsource]

This would require an update to the SourceGenerator as well and will break a lot of existing code (there are several PRs in the StyleCop repo related to this change with a lot of files changed). We can and should absolutely do this. But it isn't an easy effort. We can however still take my solution (which by the way was implemented in StyleCop almost the same way before they moved it it to the source generator: https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/f8f0b4b126f55c4619d0930c4a5aeb59f0c572a2/StyleCop.Analyzers/StyleCop.Analyzers/Lightup/IOperationWrapper.cs) as a quick fix for the allocation issue.

[andrei-epure-sonarsource]

[education] can this have an impact on the Security FE ?

[martin-strecker-sonarsource]

Yes. The FE frontend might be affected by the change from class to readonöly struct. IOperationWrapperSonar could be null before but is never null now (instead the wrapped IOperationWrapperSonar.Instance can be null now).

[andrei-epure-sonarsource]

can you check with them on slack if they have a ticket in JIRA for updating the CFG library (they should), and after this gets merged, to add a note about this expected change?

[andrei-epure-sonarsource]

from what I understand, it will impact in the sense that when doing the update they may need to change some of the code.

Could it lead to unexpected failures if some test cases are missing from the front-end?
Can you think of an example of practical code snippet where the behavior would change?

[andrei-epure-sonarsource]

And also they will need to update all the property names, I guess

[martin-strecker-sonarsource]

Can you think of an example of practical code snippet where the behavior would change?

I don't know if the FE access the IOperationWrapperSonar at all. It is in most parts used internally in the SE-engine. The biggest impact is the null behavior which results in changes likes that one in this PR:

- var successors = current.Operation == null ? ProcessBranching(current) : ProcessOperation(current);
+ var successors = current.Operation.Instance == null ? ProcessBranching(current) : ProcessOperation(current);

here

This is a very subtle change and can only be caught by unit tests. current.Operation == null is never true, but the compiler is not complaining here.

And also they will need to update all the property names, I guess

No. The public surface stays the same. It is the change from class to struct that is problematic (but needed).

Could it lead to unexpected failures if some test cases are missing from the front-end?

Yes. See above.

[andrei-epure-sonarsource]

Ok, this is worthy to mention in a JIRA ticket to keep in mind.

[andrei-epure-sonarsource]

sorry to ask silly questions, but given that I am the only volunteer reviewer, I need to understand now how the whole ShimLayer is working.

Why does having a Func reduce memory allocations? Wouldn't each property call create a new instance in memory? As opposed to using something that is cached?

Or is rather the fact that it does create an instance just for the scope of its usage, and previously the ReadCached was actually storing ALL instances, even when they were not necessary?

Do I understand well that this is a tradeoff of memory usage (drop the cache) vs cycle usage (we will recreated the same Parent instance multiple times for the same syntax node?

[andrei-epure-sonarsource]

in your perf evaluation (#8105 (comment)) I see you only mention memory and not CPU usage difference (or I am reading it wrong?)

[martin-strecker-sonarsource]

Before the IOperationWrapperSonar was a class and instantiated via the the IOperationWrapperSonar ToSonar(this IOperation) extension method. The class was used to access properties like IOperation.Parent which was introduced in a later version of Roslyn that we reference. The old solution access the properties via reflection like so:

• A static PropertyInfo of the Parent property was created in the static constructor and stored in a static field
• For each instance of IOperationWrapperSonar a reflection-based call via PropertyInfo.GetValue was made. This is very slow. The result was therefore cached in a field of the instance.

This resulted in

• An allocation of an instance of the wrapper on each call to ToSonar resulting in 160MB of total allocations (the object contains 5 fields (all object references) so one instance might be roughly 5 times 8 byte + overhead). So there are a lot of instances around.
• A slow call for the first time the property of the instance is called (the important part: the reflection call happens on each instance)

The "normal" way the wrapper work is like so:

• Create a readonly struct that wraps the IOperation in a single field (called Instance). This does not allocate on the heap at all (we do not box the wrappers anywhere). It doesn't add any overhead at all as the single field is exactly as big as the original value, so the wrapper takes as much stack space as without the wrapper.
• The access to the properties is done via a Func<IOperation, TResult> delegate. This delegate is stored in a static field and created in the static constructor. The delegate implementation is dynamically created via System.Linq.Expressions and compiled (via LambdaExpression.Compile which generates IL code via Reflection.Emit in a dynamic generated assembly). This takes a significant time but is paid only once at the very first access of the first instance (first call to ToSonar() of the first instance). Every subsequent call for any instance is as fast as IL generated by the compiler in a normal assembly. The generated implementation for that delegate is just a simple property access and looks about like so (IOperation operation) => operation.Parent;

I did not measure the performance impact on time. I don't think it is measurable in a real-world scanner run but I could give it a try in the analyzerrunner with the SymbolicExecutionRunner analyzer only.

[andrei-epure-sonarsource]

Ok... I suggest measuring the performance impact on time as well, to be sure.

Better safe than sorry.

[martin-strecker-sonarsource]

With the analyzer runner and SE only for CSVHelper I get the following results:

Allocations

18MB of 1.5 GB (11th place or 1.1%)

After: no allocations for IOperationWrapperSonar

Time savings

Time spent in PropertyInfo.GetValue before 685 ms (0.05% of overall runtime) down to 15ms after.

Before
Place 25th of most expensive methods

After

[andrei-epure-sonarsource]

LGTM