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.

Sign up for GitHub

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

Jump to bottom

SE: Caching: Cache "GetHashCode" for SymbolicValue #6968

Merged
merged 12 commits into from
Mar 27, 2023
Merged

SE: Caching: Cache "GetHashCode" for SymbolicValue #6968

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
65d1021
Add cache for SymbolicValue
martin-strecker-sonarsource Mar 21, 2023
49b5c30
Add copy constructor to reset "Lazy"
martin-strecker-sonarsource Mar 21, 2023
59cf2fb
Remove duplicate code
martin-strecker-sonarsource Mar 21, 2023
80c001e
Use nullable int instead on lazy
martin-strecker-sonarsource Mar 23, 2023
cbd6fae
Fix implementation
martin-strecker-sonarsource Mar 23, 2023
83d3aae
GetHashCode tests for SymbolicValue
martin-strecker-sonarsource Mar 23, 2023
389b423
Add more cases.
martin-strecker-sonarsource Mar 23, 2023
82134d2
Supress warning
martin-strecker-sonarsource Mar 23, 2023
ee16d48
Remove supression
martin-strecker-sonarsource Mar 23, 2023
0a5e74f
Formatting
martin-strecker-sonarsource Mar 23, 2023
bd2aa1a
Rename field
martin-strecker-sonarsource Mar 23, 2023
f3ef93d
Remove if
martin-strecker-sonarsource Mar 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Roslyn/SymbolicValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

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.

Suggested change
private int? constraintsHashCode;
private int? hashCode;


// 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)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

{
constraints = value;
constraintsHashCode = null;
}
}
}
Copy link
Contributor Author

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;
    }
}

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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).


public IEnumerable<SymbolicConstraint> AllConstraints =>
Constraints.Values;
Expand Down Expand Up @@ -83,7 +97,7 @@ public SymbolicValue WithConstraint(SymbolicConstraint constraint)
Constraints.TryGetValue(typeof(T), out var value) ? (T)value : null;

public override int GetHashCode() =>
HashCode.DictionaryContentHash(Constraints);
constraintsHashCode ??= HashCode.DictionaryContentHash(constraints);

public bool Equals(SymbolicValue other) =>
other is not null && other.Constraints.DictionaryEquals(Constraints);
Expand Down
42 changes: 42 additions & 0 deletions analyzers/tests/SonarAnalyzer.UnitTest/SymbolicExecution/Roslyn/SymbolicValueTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,5 +335,47 @@ public void RemoveEntry_Miss_Returns_Instance_Type()
var sut = SymbolicValue.Null.WithConstraint(TestConstraint.First);
sut.WithoutConstraint<DummyConstraint>().Should().BeSameAs(sut).And.HaveOnlyConstraints(ObjectConstraint.Null, TestConstraint.First);
}

[TestMethod]
public void GetHashCode_ReturnsDifferentValuesForPredefinedValues()
{
SymbolicValue.Empty.GetHashCode().Should().Be(SymbolicValue.Empty.GetHashCode());
SymbolicValue.Empty.GetHashCode().Should()
.NotBe(SymbolicValue.Null.GetHashCode()).And
.NotBe(SymbolicValue.NotNull.GetHashCode()).And
.NotBe(SymbolicValue.True.GetHashCode()).And
Copy link
Contributor

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 &&

.NotBe(SymbolicValue.False.GetHashCode());
}

[TestMethod]
public void GetHashCode_DifferentValuesAreUnique()
{
new[]
{
SymbolicValue.Empty.GetHashCode(),
SymbolicValue.NotNull.GetHashCode(),
SymbolicValue.Null.GetHashCode(),
SymbolicValue.True.GetHashCode(),
SymbolicValue.False.GetHashCode(),
SymbolicValue.Empty.WithConstraint(DummyConstraint.Dummy).GetHashCode(),
SymbolicValue.Empty.WithConstraint(DummyConstraint.Dummy).WithConstraint(TestConstraint.First).WithConstraint(ObjectConstraint.Null).GetHashCode(),
SymbolicValue.Empty.WithConstraint(DummyConstraint.Dummy).WithConstraint(TestConstraint.First).WithConstraint(ObjectConstraint.NotNull).GetHashCode(),
}.Should().OnlyHaveUniqueItems();
}

[TestMethod]
public void GetHashCode_UncachedValues()
{
var baseConstraint = SymbolicValue.Empty
.WithConstraint(DummyConstraint.Dummy)
.WithConstraint(TestConstraint.First)
.WithConstraint(ObjectConstraint.Null)
.WithConstraint(BoolConstraint.True);
baseConstraint.GetHashCode().Should().Be(baseConstraint.GetHashCode());
var similar = baseConstraint.WithoutConstraint(DummyConstraint.Dummy).WithConstraint(DummyConstraint.Dummy);
similar.Should().NotBeSameAs(baseConstraint);
similar.GetHashCode().Should().Be(baseConstraint.GetHashCode());
similar.Equals(baseConstraint).Should().BeTrue();
}
}
}