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: Caching: Cache "GetHashCode" for SymbolicValue #6968

Merged
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

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 T Constraint<T>() where T : SymbolicConstraint =>
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
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();
}
}
}