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

Optimize BeEquivalentTo #1939

Merged
merged 6 commits into from Jun 2, 2022
Merged

Optimize BeEquivalentTo #1939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
373a6ad
Lazily construct the equivalency options summary
jnyrup May 27, 2022
6c5db11
Avoid repeatedly constructing lambda
jnyrup May 27, 2022
981570b
Avoid allocations in BecauseOf
jnyrup May 27, 2022
8da904f
Avoid allocations in ConversionSelector
jnyrup May 27, 2022
370c674
Avoid/cache string allocations
jnyrup May 27, 2022
a741e66
Verify that failure message includes options
jnyrup May 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions Src/FluentAssertions/Collections/GenericCollectionAssertions.cs
View file
Open in desktop
Expand Up @@ -845,7 +845,7 @@ public AndConstraint<TAssertions> Contain(IEnumerable<T> expected, string becaus

using (var scope = new AssertionScope())
{
scope.AddReportable("configuration", options.ToString());
scope.AddReportable("configuration", () => options.ToString());

foreach (T actualItem in Subject)
{
Expand Down Expand Up @@ -2199,7 +2199,7 @@ public AndConstraint<TAssertions> NotContain(IEnumerable<T> unexpected, string b
Execute.Assertion
.BecauseOf(because, becauseArgs)
.WithExpectation("Expected {context:collection} {0} not to contain equivalent of {1}{reason}, ", Subject, unexpected)
.AddReportable("configuration", options.ToString());
.AddReportable("configuration", () => options.ToString());

if (foundIndices.Count == 1)
{
Expand Down
26 changes: 19 additions & 7 deletions Src/FluentAssertions/Equivalency/ConversionSelector.cs
View file
Open in desktop
Expand Up @@ -27,8 +27,19 @@ public ConversionSelectorRule(Func<IObjectInfo, bool> predicate, string descript
}
}

private List<ConversionSelectorRule> inclusions = new();
private List<ConversionSelectorRule> exclusions = new();
private readonly List<ConversionSelectorRule> inclusions;
private readonly List<ConversionSelectorRule> exclusions;

public ConversionSelector()
: this(new List<ConversionSelectorRule>(), new List<ConversionSelectorRule>())
{
}

private ConversionSelector(List<ConversionSelectorRule> inclusions, List<ConversionSelectorRule> exclusions)
{
this.inclusions = inclusions;
this.exclusions = exclusions;
}

public void IncludeAll()
{
Expand All @@ -55,6 +66,11 @@ public void Exclude(Expression<Func<IObjectInfo, bool>> predicate)

public bool RequiresConversion(Comparands comparands, INode currentNode)
{
if (inclusions.Count == 0)
{
return false;
}

var objectInfo = new ObjectInfo(comparands, currentNode);

return inclusions.Any(p => p.Predicate(objectInfo)) && !exclusions.Any(p => p.Predicate(objectInfo));
Expand Down Expand Up @@ -84,10 +100,6 @@ public override string ToString()

public ConversionSelector Clone()
{
return new ConversionSelector
{
inclusions = new List<ConversionSelectorRule>(inclusions),
exclusions = new List<ConversionSelectorRule>(exclusions),
};
return new ConversionSelector(new List<ConversionSelectorRule>(inclusions), new List<ConversionSelectorRule>(exclusions));
}
}
6 changes: 4 additions & 2 deletions Src/FluentAssertions/Equivalency/EquivalencyValidator.cs
View file
Open in desktop
@@ -1,4 +1,5 @@
using System;
using FluentAssertions.Equivalency.Tracing;
using FluentAssertions.Execution;

namespace FluentAssertions.Equivalency;
Expand All @@ -15,7 +16,7 @@ public void AssertEquality(Comparands comparands, EquivalencyValidationContext c
using var scope = new AssertionScope();

scope.AssumeSingleCaller();
scope.AddReportable("configuration", context.Options.ToString());
scope.AddReportable("configuration", () => context.Options.ToString());
scope.BecauseOf(context.Reason);

RecursivelyAssertEquality(comparands, context);
Expand Down Expand Up @@ -64,12 +65,13 @@ private void RunStepsUntilEquivalencyIsProven(Comparands comparands, IEquivalenc
{
using var _ = context.Tracer.WriteBlock(node => node.Description);

Func<IEquivalencyStep, GetTraceMessage> getMessage = step => _ => $"Equivalency was proven by {step.GetType().Name}";
Copy link
Member Author

Choose a reason for hiding this comment

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

In develop the lambda is constructed in each iteration no matter if we enter the if or not.

See the difference on
sharplab

foreach (IEquivalencyStep step in AssertionOptions.EquivalencyPlan)
{
var result = step.Handle(comparands, context, this);
if (result == EquivalencyResult.AssertionCompleted)
{
context.Tracer.WriteLine(_ => $"Equivalency was proven by {step.GetType().Name}");
context.Tracer.WriteLine(getMessage(step));
return;
}
}
Expand Down
34 changes: 29 additions & 5 deletions Src/FluentAssertions/Equivalency/Node.cs
View file
Open in desktop
@@ -1,5 +1,6 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;
using FluentAssertions.Common;
Expand All @@ -13,15 +14,35 @@ public class Node : INode
{
private static readonly Regex MatchFirstIndex = new(@"^\[\d+\]$");

private string path;
private string name;
private string pathAndName;

public GetSubjectId GetSubjectId { get; protected set; } = () => string.Empty;

public Type Type { get; protected set; }

public string Path { get; protected set; }
public string Path
{
get => path;
protected set
{
path = value;
pathAndName = null;
}
}

public string PathAndName => Path.Combine(Name);
public string PathAndName => pathAndName ??= Path.Combine(Name);
Copy link
Member

Choose a reason for hiding this comment

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

Is this really that expensive?

Copy link
Member Author

@jnyrup jnyrup May 29, 2022 •
edited

Choose a reason for hiding this comment

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

Expensiveness might be a relative term, as it has to compared against code maintainability.
The raw numbers in terms of performance are listed in the last benchmark.

The operation of concatenating strings (of course) depends on the length of the member name and how far down the member path we are.
The benchmarks performed have quite short type and member names, so a more realistic example with more descriptive/longer names would benefit even more from this.

Many of the calls to PathAndName are avoided in 8eeb20c as ObjectInfo.ctor calls PathAndName and also by using Path and Name separately in Node.Equals and Node.GetHashCode.

The remaining calls to PathAndName comes from

  • Node.Depth
  • Node.IsRoot
  • EquivalencyValidationContext.IsCyclicReference
  • Property.ctor

For this example below the caching of PathAndName reduces the number of allocating calls to PathName from 90 to 20.

var subject = new MyClass[]
{
    new () { Value = 1, Inner = new () { Value = 2 } },
    new () { Value = 1, Inner = new () { Value = 2 } },
    new () { Value = 1, Inner = new () { Value = 2 } },
    new () { Value = 1, Inner = new () { Value = 2 } },
    new () { Value = 1, Inner = new () { Value = 2 } },
};
var expected = = new MyClass[]
{
    new () { Value = 1, Inner = new () { Value = 2 } },
    new () { Value = 1, Inner = new () { Value = 2 } },
    new () { Value = 1, Inner = new () { Value = 2 } },
    new () { Value = 1, Inner = new () { Value = 2 } },
    new () { Value = 1, Inner = new () { Value = 2 } },
};

subject.Should().BeEquivalentTo(expected);


public string Name { get; set; }
public string Name
{
get => name;
set
{
name = value;
pathAndName = null;
}
}

public virtual string Description => $"{GetSubjectId().Combine(PathAndName)}";

Expand Down Expand Up @@ -109,14 +130,17 @@ public override bool Equals(object obj)
return Equals((Node)obj);
}

private bool Equals(Node other) => Type == other.Type && PathAndName == other.PathAndName;
private bool Equals(Node other) => (Type, Name, Path) == (other.Type, other.Name, other.Path);
Copy link
Member Author

Choose a reason for hiding this comment

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

The change from two to three branches is the cause of the remaining decrease in branch coverage.


public override int GetHashCode()
{
unchecked
{
#pragma warning disable CA1307
return (Type.GetHashCode() * 397) ^ PathAndName.GetHashCode();
int hashCode = Type.GetHashCode();
hashCode = (hashCode * 397) + Path.GetHashCode();
hashCode = (hashCode * 397) + Name.GetHashCode();
return hashCode;
}
}

Expand Down
16 changes: 9 additions & 7 deletions Src/FluentAssertions/Equivalency/Steps/StructuralEqualityEquivalencyStep.cs
View file
Open in desktop
Expand Up @@ -15,22 +15,24 @@ public class StructuralEqualityEquivalencyStep : IEquivalencyStep
return EquivalencyResult.ContinueWithNext;
}

bool expectationIsNotNull = AssertionScope.Current
.ForCondition(comparands.Expectation is not null)
if (comparands.Expectation is null)
{
AssertionScope.Current
.BecauseOf(context.Reason)
.FailWith(
"Expected {context:subject} to be <null>{reason}, but found {0}.",
comparands.Subject);

bool subjectIsNotNull = AssertionScope.Current
.ForCondition(comparands.Subject is not null)
}
else if (comparands.Subject is null)
{
AssertionScope.Current
.BecauseOf(context.Reason)
.FailWith(
"Expected {context:object} to be {0}{reason}, but found {1}.",
comparands.Expectation,
comparands.Subject);

if (expectationIsNotNull && subjectIsNotNull)
}
else
{
IMember[] selectedMembers = GetMembersFromExpectation(context.CurrentNode, comparands, context.Options).ToArray();
if (context.CurrentNode.IsRoot && !selectedMembers.Any())
Expand Down
16 changes: 16 additions & 0 deletions Tests/FluentAssertions.Equivalency.Specs/CyclicReferencesSpecs.cs
View file
Open in desktop
Expand Up @@ -158,6 +158,22 @@ public void
act.Should().NotThrow();
}

[Fact]
public void Allowing_infinite_recursion_is_described_in_the_failure_message()
{
// Arrange
var recursiveClass1 = new ClassWithFiniteRecursiveProperty(1);
var recursiveClass2 = new ClassWithFiniteRecursiveProperty(2);

// Act
Action act = () => recursiveClass1.Should().BeEquivalentTo(recursiveClass2,
options => options.AllowingInfiniteRecursion());

// Assert
act.Should().Throw<XunitException>()
.WithMessage("*Recurse indefinitely*");
}

[Fact]
public void When_injecting_a_null_config_to_BeEquivalentTo_it_should_throw()
{
Expand Down
96 changes: 96 additions & 0 deletions Tests/FluentAssertions.Equivalency.Specs/SelectionRulesSpecs.cs
View file
Open in desktop
Expand Up @@ -51,6 +51,102 @@ public void When_specific_properties_have_been_specified_it_should_ignore_the_ot
act.Should().NotThrow();
}

[Fact]
public void A_member_included_by_path_is_described_in_the_failure_message()
{
// Arrange
var subject = new
{
Name = "John"
};

var customer = new
{
Name = "Jack"
};

// Act
Action act = () => subject.Should().BeEquivalentTo(customer, options => options
.Including(d => d.Name));

// Assert
act.Should().Throw<XunitException>()
.WithMessage("*Include*Name*");
}

[Fact]
public void A_member_included_by_predicate_is_described_in_the_failure_message()
{
// Arrange
var subject = new
{
Name = "John"
};

var customer = new
{
Name = "Jack"
};

// Act
Action act = () => subject.Should().BeEquivalentTo(customer, options => options
.Including(ctx => ctx.Path == "Name"));

// Assert
act.Should().Throw<XunitException>()
.WithMessage("*Include member when*Name*");
}

[Fact]
public void A_member_excluded_by_path_is_described_in_the_failure_message()
{
// Arrange
var subject = new
{
Name = "John",
Age = 13
};

var customer = new
{
Name = "Jack",
Age = 37
};

// Act
Action act = () => subject.Should().BeEquivalentTo(customer, options => options
.Excluding(d => d.Age));

// Assert
act.Should().Throw<XunitException>()
.WithMessage("*Exclude*Age*");
}

[Fact]
public void A_member_excluded_by_predicate_is_described_in_the_failure_message()
{
// Arrange
var subject = new
{
Name = "John",
Age = 13
};

var customer = new
{
Name = "Jack",
Age = 37
};

// Act
Action act = () => subject.Should().BeEquivalentTo(customer, options => options
.Excluding(ctx => ctx.Path == "Age"));

// Assert
act.Should().Throw<XunitException>()
.WithMessage("*Exclude member when*Age*");
}

[Fact]
public void When_a_predicate_for_properties_to_include_has_been_specified_it_should_ignore_the_other_properties()
{
Expand Down