SE: Maintenance: Cache GetHashCode for ProgramState #6983

martin-strecker-sonarsource · 2023-03-23T18:58:23Z

Part of #6964

martin-strecker-sonarsource · 2023-03-23T19:07:05Z

Before: 45.7 GB allocations
After: 44.4 GB allocations
Note: This PR and the baseline for comparison both don't have the cache of the HashCode of SymbolicValue included (#6968).

Comparison (left: before, right: after)

Enumerator halved in allocation count and GBs

pavel-mikula-sonarsource

Let's try to wrap it to remove duplications.

pavel-mikula-sonarsource · 2023-03-24T15:20:30Z

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/ProgramState.cs

+        private int? exceptionsHashCode;
+        private int? hashCode;
+
+        // Current SymbolicValue result of a given operation


You can remove this. I don't find it useful anymore when the engine has evolved and the ProgramState is well understood.

Suggested change

// Current SymbolicValue result of a given operation

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/ProgramState.cs

pavel-mikula-sonarsource · 2023-03-24T15:24:19Z

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/ProgramState.cs

-                HashCode.DictionaryContentHash(CaptureOperation),
-                HashCode.EnumerableContentHash(PreservedSymbols),
-                HashCode.EnumerableContentHash(Exceptions));
+            hashCode ??= HashCode.Combine(


Can you measure how many times we create ProgramState without ever calling getHashCode? In theory, we could do this math at creation time.. or at least inside the wrapped type for the sub-items.

This is the result for the NullPointerDereference_Roslyn_CS test case:

Count of GetHashcode calls ProgramState unique objects count Description

0 5805 GetHashcode was never called

1 4282 GetHashcode was called exactly once

145 1 GetHashcode was called 145 times for 1 instance

So we have more than half of the objects, where GetHashCode is never called, and the other half, where GetHashCode is called once. We have one case, where GetHashCode is called 145 times. This is most likely the ProgramState.Empty instance.
This shows that

it is more important to cache the single hashcodes than the total hashcode

it is important to lazy evaluate the hashcode.

Here is the set of values from NullPointerDereference_Roslyn_CSharp8. It shows about the same results:

Count of GetHashcode calls ProgramState unique objects count

0 1298

1 772

32 1

I did similar stats against Nancy project:
0x 145696
1x: 124010
2x: 13
3x: 18
3520x: 1 - And I confirmed it's the "Empty" one.

To me, it does not make sense to cache it. There's nothing to win, there's no cache-hit.

The only shortcut we can take is to return 0 when reference equals to Empty.

The saving is that in 124.010 cases we do calculate most likely a single hashcode for a single ImmutableDictionary instead of 5.

Ok. If the main gain is on the partial sub-results, what about removing hashCode itself?

It's calculation should be rather cheap. And it's not reused much. It will also simplify the inits.

As an alternative solution, we can make the hashcode implementation allocations free (my main concern here). I did so in #7012. There you can find an allocation comparison between the base-line, this PR (caching), and an allocation-free (#7012) implementation. I would recommend merging #7012 in any case and we may also merge this PR on top of it (which would additionally save some CPU cycles).

Let's merge the other one. And measure the impact of this after.

pavel-mikula-sonarsource

Given the measurements, this change doesn't make sense. We can take shortcut on the Empty instance.

pavel-mikula-sonarsource

Probably last set of questions

pavel-mikula-sonarsource · 2023-03-29T14:49:39Z

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/ProgramState.cs

-                HashCode.DictionaryContentHash(CaptureOperation),
-                HashCode.EnumerableContentHash(PreservedSymbols),
-                HashCode.EnumerableContentHash(Exceptions));
+            hashCode ??= HashCode.Combine(


Ok. If the main gain is on the partial sub-results, what about removing hashCode itself?

It's calculation should be rather cheap. And it's not reused much. It will also simplify the inits.

pavel-mikula-sonarsource · 2023-03-29T14:50:12Z

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/ProgramState.cs

-                HashCode.DictionaryContentHash(CaptureOperation),
-                HashCode.EnumerableUnorderedContentHash(PreservedSymbols),
-                HashCode.EnumerableOrderedContentHash(Exceptions));
+            hashCode ??= HashCode.Combine(


We'd need a performance comment here, explaining the overall conclusion of this PR... and why every new field should have its HashCode

pavel-mikula-sonarsource · 2023-03-29T14:53:44Z

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/ProgramState.cs

+        private int? hashCode;
+
+        private ImmutableDictionary<IOperation, SymbolicValue> OperationValue { get => operationValue; init => SetCachedHashCodeField(value, ref operationValue, ref operationValueHashCode); }
+        private ImmutableDictionary<ISymbol, SymbolicValue> SymbolValue { get => symbolValue; init => SetCachedHashCodeField(value, ref symbolValue, ref symbolValueHashCode); }
        private ImmutableDictionary<int, int> VisitCount { get; init; }


Let's put VisitCount last, to make it more regular. Also in the copy constructor

…bols in NonNullableValueTypeCheck (#6995)

…le dict (#7012)

sonarcloud · 2023-03-31T17:07:21Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarcloud · 2023-03-31T17:11:25Z

SonarCloud Quality Gate failed.

1 Bug
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

100.0% Coverage
0.0% Duplication

martin-strecker-sonarsource · 2023-03-31T17:25:20Z

Based on the merged allocation free hashcode calculation, we see these figures:

ProgramState has more fields now and is bigger and therefore takes more MB.

The changes are minimal and within the margin of error

Run	Byte	object count
Base line	39.451.830	692.333
cached	39.886.055	691.585

I haven't really measured the impact on runtime as this requires more than one run but the analyzer runner reported these numbers for the two runs:

With Cache: 159365,7515 ms
Base line: 165859,5391 ms

I would suggest closing the PR (as the changes are really ugly) and instead paying more attention to System.Collections.Immutable.SortedInt32KeyNode<> which takes the crown in allocations (object count and MB). I described in #6964 under Rethink ProgramState.AddVisit what we can do there.

pavel-mikula-sonarsource · 2023-04-06T07:41:10Z

Makes sense to close it. The root of the problem is gone.

martin-strecker-sonarsource added the Sprint: SE Short-lived* label for epic MMF-3077 *troll label Mar 23, 2023

github-actions bot assigned martin-strecker-sonarsource Mar 23, 2023

github-actions bot added this to In progress in Best Kanban Mar 23, 2023

martin-strecker-sonarsource mentioned this pull request Mar 23, 2023

SE: Add caching for reusable data structures #6964

Open

16 tasks

martin-strecker-sonarsource requested a review from pavel-mikula-sonarsource March 23, 2023 19:13

github-actions bot assigned pavel-mikula-sonarsource and unassigned martin-strecker-sonarsource Mar 23, 2023

github-actions bot moved this from In progress to Review in progress in Best Kanban Mar 23, 2023

pavel-mikula-sonarsource requested changes Mar 24, 2023

View reviewed changes

github-actions bot assigned martin-strecker-sonarsource and unassigned pavel-mikula-sonarsource Mar 24, 2023

github-actions bot moved this from Review in progress to In progress in Best Kanban Mar 24, 2023

martin-strecker-sonarsource force-pushed the Martin/SE_6964_04_CacheGetHashCodeProgramState branch from e4d92c6 to a918e14 Compare March 27, 2023 11:19

martin-strecker-sonarsource requested a review from pavel-mikula-sonarsource March 27, 2023 12:01

github-actions bot assigned pavel-mikula-sonarsource and unassigned martin-strecker-sonarsource Mar 27, 2023

github-actions bot moved this from In progress to Review in progress in Best Kanban Mar 27, 2023

pavel-mikula-sonarsource force-pushed the feature/SE branch from 119d33f to ad802d1 Compare March 28, 2023 10:43

pavel-mikula-sonarsource requested changes Mar 28, 2023

View reviewed changes

pavel-mikula-sonarsource moved this from Review in progress to In progress in Best Kanban Mar 28, 2023

pavel-mikula-sonarsource assigned martin-strecker-sonarsource and unassigned pavel-mikula-sonarsource Mar 28, 2023

martin-strecker-sonarsource force-pushed the Martin/SE_6964_04_CacheGetHashCodeProgramState branch from bc50906 to 771b8bf Compare March 28, 2023 17:04

martin-strecker-sonarsource requested a review from pavel-mikula-sonarsource March 28, 2023 17:14

github-actions bot assigned pavel-mikula-sonarsource and unassigned martin-strecker-sonarsource Mar 28, 2023

github-actions bot moved this from In progress to Review in progress in Best Kanban Mar 28, 2023

pavel-mikula-sonarsource requested changes Mar 29, 2023

View reviewed changes

github-actions bot unassigned pavel-mikula-sonarsource Mar 29, 2023

Tim-Pohlmann and others added 7 commits March 31, 2023 13:31

Fix S3655 FN: VB implicit conversions (#6991)

01a1787

S3655: Fields with methods, local functions, yield and async (#6981)

aa64a7c

S3655: Use IsNonNullableValueType (#6989)

daed2c1

Fix S2259 FP: Do not track symbol via TryCast (#6992)

f624ed5

SE - Non-nullable value types: Set NotNull Constraint for tracked Sym…

35a86f0

…bols in NonNullableValueTypeCheck (#6995)

Fix S3900 FN: Conversion operations (#7002)

9be9ba3

S3655: Boxing (#6996)

d6211d9

pavel-mikula-sonarsource force-pushed the feature/SE branch from d1fa962 to d6211d9 Compare March 31, 2023 11:32

pavel-mikula-sonarsource and others added 13 commits March 31, 2023 14:01

SE: Support ReferenceEquals (#7005)

79a26df

Fix S3900 and S2259 FN: Support method dereference (#7011)

b4d6355

S3655: Static properties and fields called Value (#7000)

12c8b40

S3900: Change parameter dereference check to top-down (#7004)

60dc3c5

SE: Maintenance: Allocation free hash code implementation for immutab…

508b279

…le dict (#7012)

Add Caching for ProgramState hashcodes

baf2441

Adopt copy constructor

53470a9

Remove toArray reset.

a82f7d1

Move private method to buttom

980593d

Delegate ctor

69dcecd

Remove comment

983f6ed

Null coalescing in copy constructor

6664bf6

Remove ContentDictionary

8567e1c

martin-strecker-sonarsource force-pushed the Martin/SE_6964_04_CacheGetHashCodeProgramState branch from 2dce265 to 8567e1c Compare March 31, 2023 16:58

Base automatically changed from feature/SE to master April 4, 2023 13:05

pavel-mikula-sonarsource closed this Apr 6, 2023

Best Kanban automation moved this from In progress to Done Apr 6, 2023

pavel-mikula-sonarsource deleted the Martin/SE_6964_04_CacheGetHashCodeProgramState branch April 6, 2023 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SE: Maintenance: Cache GetHashCode for ProgramState #6983

SE: Maintenance: Cache GetHashCode for ProgramState #6983

martin-strecker-sonarsource commented Mar 23, 2023

martin-strecker-sonarsource commented Mar 23, 2023 •

edited

Loading

pavel-mikula-sonarsource left a comment

pavel-mikula-sonarsource Mar 24, 2023

pavel-mikula-sonarsource Mar 24, 2023

martin-strecker-sonarsource Mar 27, 2023 •

edited

Loading

pavel-mikula-sonarsource Mar 28, 2023

martin-strecker-sonarsource Mar 28, 2023

pavel-mikula-sonarsource Mar 29, 2023

martin-strecker-sonarsource Mar 30, 2023 •

edited

Loading

pavel-mikula-sonarsource Mar 31, 2023

pavel-mikula-sonarsource left a comment

pavel-mikula-sonarsource left a comment

pavel-mikula-sonarsource Mar 29, 2023

pavel-mikula-sonarsource Mar 29, 2023

pavel-mikula-sonarsource Mar 29, 2023

sonarcloud bot commented Mar 31, 2023

sonarcloud bot commented Mar 31, 2023

martin-strecker-sonarsource commented Mar 31, 2023 •

edited

Loading

pavel-mikula-sonarsource commented Apr 6, 2023

Count of GetHashcode calls	ProgramState unique objects count	Description
0	5805	GetHashcode was never called
1	4282	GetHashcode was called exactly once
145	1	GetHashcode was called 145 times for 1 instance

SE: Maintenance: Cache GetHashCode for ProgramState #6983

SE: Maintenance: Cache GetHashCode for ProgramState #6983

Conversation

martin-strecker-sonarsource commented Mar 23, 2023

martin-strecker-sonarsource commented Mar 23, 2023 • edited Loading

pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-strecker-sonarsource Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-strecker-sonarsource Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 31, 2023

sonarcloud bot commented Mar 31, 2023

martin-strecker-sonarsource commented Mar 31, 2023 • edited Loading

pavel-mikula-sonarsource commented Apr 6, 2023

martin-strecker-sonarsource commented Mar 23, 2023 •

edited

Loading

martin-strecker-sonarsource Mar 27, 2023 •

edited

Loading

martin-strecker-sonarsource Mar 30, 2023 •

edited

Loading

martin-strecker-sonarsource commented Mar 31, 2023 •

edited

Loading