-
Notifications
You must be signed in to change notification settings - Fork 229
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: Caching: Cache "GetHashCode" for SymbolicValue #6968
SE: Caching: Cache "GetHashCode" for SymbolicValue #6968
Conversation
@@ -37,6 +39,11 @@ public sealed record SymbolicValue | |||
public IEnumerable<SymbolicConstraint> AllConstraints => | |||
Constraints.Values; | |||
|
|||
public SymbolicValue() | |||
{ | |||
hashCode = new(() => HashCode.DictionaryContentHash(Constraints), LazyThreadSafetyMode.ExecutionAndPublication); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For LazyThreadSafetyMode
see https://learn.microsoft.com/en-us/dotnet/api/system.threading.lazythreadsafetymode?view=net-8.0
2a0e127
to
f4f2e29
Compare
cb2c9f8
to
703c93e
Compare
So it reduced the number of iterations on your dictionary with a factor two? That's nice. Did you create a benchmark too? |
No. This is done with the AnalyzerRunner and the Visual Studio diagnostics tool. I may add a markdown file to our docs on how to do that. It might come in handy as we have quite some of these optimizations to do and this helps with guiding in the right direction. |
703c93e
to
59cf2fb
Compare
private readonly ImmutableDictionary<Type, SymbolicConstraint> constraints = ImmutableDictionary<Type, SymbolicConstraint>.Empty; | ||
private int? constraintsHashCode; | ||
|
||
// SymbolicValue can have only one constraint instance of specific type at a time | ||
private ImmutableDictionary<Type, SymbolicConstraint> Constraints { get; init; } = ImmutableDictionary<Type, SymbolicConstraint>.Empty; | ||
private ImmutableDictionary<Type, SymbolicConstraint> Constraints | ||
{ | ||
get => constraints; | ||
init | ||
{ | ||
if (constraints != value) | ||
{ | ||
constraints = value; | ||
constraintsHashCode = null; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also
sonar-dotnet/analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/ProgramState.cs
Lines 138 to 143 in f3c2ea1
HashCode.Combine( | |
HashCode.DictionaryContentHash(OperationValue), | |
HashCode.DictionaryContentHash(SymbolValue), | |
HashCode.DictionaryContentHash(CaptureOperation), | |
HashCode.EnumerableContentHash(PreservedSymbols), | |
HashCode.EnumerableContentHash(Exceptions)); |
and I want to use the same pattern there as well. There it is more important to reset the cached hashcode in the init setter. There we can do this, to make the declaration more compact:
private readonly ImmutableDictionary<IOperation, SymbolicValue> operationValue;
private int? operationValueHashCode;
private ImmutableDictionary<IOperation, SymbolicValue> OperationValue
{
get => operationValue;
init => SetHashCodeCachedField(value, ref operationValue, ref operationValueHashCode);
}
private static void SetHashCodeCachedField<T>(T value, ref T backingField, ref int? backingFieldHashCode) where T: class
{
if (value != backingField)
{
backingField = value;
backingFieldHashCode = null;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The init
and ref
trick sounds good in principle.
backingFieldHashCode
should not be a parameter, the method can just reset it directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SetHashCodeCachedField
method doesn't make sense in this class. It will come in handy in ProgramState
. I just wanted to make sure we do the caching in both classes the same way, so we don't solve the same problem differently in these highly related classes. Slight differences like this should be fine though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's also my understanding that it should not be here, because it's not needed (yet).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a small rename before merging
@@ -35,8 +35,22 @@ public sealed record SymbolicValue | |||
public static readonly SymbolicValue True = NotNull.WithConstraint(BoolConstraint.True); | |||
public static readonly SymbolicValue False = NotNull.WithConstraint(BoolConstraint.False); | |||
|
|||
private readonly ImmutableDictionary<Type, SymbolicConstraint> constraints = ImmutableDictionary<Type, SymbolicConstraint>.Empty; | |||
private int? constraintsHashCode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is and will be hash code for the entire class. No matter how much mess we add.
private int? constraintsHashCode; | |
private int? hashCode; |
private readonly ImmutableDictionary<Type, SymbolicConstraint> constraints = ImmutableDictionary<Type, SymbolicConstraint>.Empty; | ||
private int? constraintsHashCode; | ||
|
||
// SymbolicValue can have only one constraint instance of specific type at a time | ||
private ImmutableDictionary<Type, SymbolicConstraint> Constraints { get; init; } = ImmutableDictionary<Type, SymbolicConstraint>.Empty; | ||
private ImmutableDictionary<Type, SymbolicConstraint> Constraints | ||
{ | ||
get => constraints; | ||
init | ||
{ | ||
if (constraints != value) | ||
{ | ||
constraints = value; | ||
constraintsHashCode = null; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The init
and ref
trick sounds good in principle.
backingFieldHashCode
should not be a parameter, the method can just reset it directly.
.NotBe(SymbolicValue.Null.GetHashCode()).And | ||
.NotBe(SymbolicValue.NotNull.GetHashCode()).And | ||
.NotBe(SymbolicValue.True.GetHashCode()).And |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put the .And
at the beginning of the line. LIke &&
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM
get => constraints; | ||
init | ||
{ | ||
if (constraints != value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition is not covered when values are the same. We don't use it that way. And we actually shouldn't, as we should attempt no-operation when possible.
Do we really need the condition then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't hurt to keep it, IMO. It allows doing something like, e.g. with Constraints = someCond ? Empty : Constraints
. I could add a test case to increase test coverage if that is a concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the condition. It's one more additional Equals
check on a hot path that doesn't need to be there.
And so far, we didn't need it. And it's against the intended usage of the property. We can always add it back if we need to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine with me.
analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/SymbolicValue.cs
Outdated
Show resolved
Hide resolved
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
Part of #6964