Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SE: Pass states per stack #7059

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

martin-strecker-sonarsource
Copy link
Contributor

Part of #6964

ProgramState[] and SymbolicContext[] are the main vehicles to branch a state. When processing an ExplodedNode (which contains a single ProgramState) we end up with these states:

  • Almost always the ExplodedNode is transferred to a single ExplodedNode (with the same or a changed state).
  • Sometimes two ExplodedNodes which represent a simple branching
  • No ExplodedNode (representing e.g. unreachable code)
  • In rare cases a single ExplodedNode is transferred to more than two ExplodedNode

The engine needs to work with all the cases and so an array is used to represent the branchings. This results in a lot of allocations of small arrays (1 or 2 elements) on the heap. This PR replaces the array with a struct which is optimized for the 0, 1, and 2 elements cases. It passes the states on the stack. It needs to reserve three pointers on the stack to do so (this increases the stack space used. The array needed a single pointer on the stack). The three pointers are used for

  • first
  • second
  • others[]

The result is a reduction in allocation in total by ~5%. It eliminates SymbolicContext[] usage entirely (down to 0
from 17.820) and reduces the allocation count by 35.000. SymbolicContext[] was the fourth most allocated object before.

The benefit (reduced GC pressure) comes at the cost of more expensive method calls (more stack space, more data transferred in a call). Because of this, a runtime performance measurement needs to be done before merging the PR.

image

Measure Baseline This PR Change
No of objects 654.817 619.815 94,65 %
Bytes 38.056.351 36.849.170 96,82 %

As a result of this change, the distinguishing between IMultiProcessor and ISimpleProcessor can be resolved. There is also no need to distinguish between PreProcess and PreProcessSimple (same for Post of course) in SymbolicCheck anymore. Both changes simplify the usage of the engine.

@sonarcloud
Copy link

sonarcloud bot commented Apr 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Apr 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 12 Code Smells

96.7% 96.7% Coverage
0.0% 0.0% Duplication

@Tim-Pohlmann Tim-Pohlmann added this to To do in Best Kanban Jul 10, 2023
@Tim-Pohlmann Tim-Pohlmann removed this from To do in Best Kanban Aug 21, 2023
@Tim-Pohlmann Tim-Pohlmann added Type: Improvement Making existing code better. Area: CFG/SE CFG and SE related issues. Area: VB.NET VB.NET rules related issues. Area: C# C# rules related issues. and removed Sprint: SE Short-lived* label for epic MMF-3077 *troll labels Aug 21, 2023
@sonarcloud
Copy link

sonarcloud bot commented Sep 27, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Sep 27, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

96.9% 96.9% Coverage
0.0% 0.0% Duplication

@martin-strecker-sonarsource
Copy link
Contributor Author

Savings for CSVHelper executed with the AnalyzerRunner:

Sorted by Bytes:
image

Sorted by object count
image

@martin-strecker-sonarsource
Copy link
Contributor Author

Real world impact (Akka.Net SonarWay):

Total: No measurable impact (SE contributes 5% of total allocations)
SE (SymbolicExecutionRunnerBase.AnalyzeRoslyn):
Before: 1.257 MB
After: 1.028 MB

Before
image

After
image

@martin-strecker-sonarsource martin-strecker-sonarsource marked this pull request as ready for review October 4, 2023 13:01
@martin-strecker-sonarsource martin-strecker-sonarsource added Type: Performance It takes too long. and removed Type: Improvement Making existing code better. labels Jan 7, 2024
Copy link

sonarcloud bot commented Feb 1, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link

sonarcloud bot commented Feb 1, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

9 New issues

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

@Tim-Pohlmann Tim-Pohlmann added the Sprint: SE Short-lived* label for epic MMF-3077 *troll label Feb 5, 2024
@Tim-Pohlmann Tim-Pohlmann added this to To do in Best Kanban Feb 5, 2024
@@ -28,7 +28,7 @@ public class SymbolicCheck
/// <summary>
/// Stop processing this branch of the exploded graph. There will be no follow up states.
/// </summary>
protected static readonly ProgramState[] EmptyStates = Array.Empty<ProgramState>();
protected static readonly ProgramStates EmptyStates = new();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be removed. Accessing the field has value semantics and a copy is created (we see a ldsfld (copies the value for value types) instead of a ldsflda (creates a reference to the field) in the generated IL).

We should just new() as we do in all the other places as well.

@Tim-Pohlmann Tim-Pohlmann removed the Sprint: SE Short-lived* label for epic MMF-3077 *troll label Feb 14, 2024
@Tim-Pohlmann Tim-Pohlmann removed this from To do in Best Kanban Feb 14, 2024
Copy link

sonarcloud bot commented Apr 24, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link

sonarcloud bot commented Apr 24, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
9 New issues

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Area: CFG/SE CFG and SE related issues. Area: VB.NET VB.NET rules related issues. Type: Performance It takes too long.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants