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

EquivalencyValidator performance fixes #935

Merged
merged 2 commits into from Nov 20, 2018

Conversation

pentp
Copy link
Contributor

@pentp pentp commented Oct 2, 2018

This fixes GenericDictionaryEquivalencyStep and GenericEnumerableEquivalencyStep performance problems caused by the use of uncached LambdaCompiler.Compile which is very expensive.
Related to #475.

This makes ObjectAssertions.BeEquivalentTo about 4.3x faster in our test projects.

@jnyrup
Copy link
Member

@jnyrup jnyrup commented Oct 4, 2018

Thanks for taking a look at this part of the code base and submitting a PR.
I find it a bit difficult to review the changes, as they cover many different parts, more than just replacing the LambdaCompiler.

Can you comment more the effects the individual changes had?
E.g. you added several conditionals to avoid calls to ForCondition and I'm interested in the real world benefits of these.

What were the absolute improvements on your tests?

@adamvoss As far as I could trace this back, you authored the initial implementation of this.
Do you recall any considerations in picking LambdaCompiler approach over MakeGenericMethod?

@pentp
Copy link
Contributor Author

@pentp pentp commented Oct 4, 2018

The conditionals before ForCondition calls are just short-circuiting the check and FailWith - this avoids a bunch of unnecessary work for the common case.
ObjectReference changes are also optimizations for the common case (no cyclic references).

For one class of tests the absolute improvement was from 17 to 13 seconds, but I don't have data for all our tests (this includes everything, e.g. starting up testhost.exe). BeEquivalentTo improved from 5391ms to 1248ms.

Copy link
Member

@jnyrup jnyrup left a comment

As you see from my comments, I'm really interested in what differences the different parts make.
Do you have a test case you can share?

For inspiration have a look at #817 (comment) where I extracted the relevant changes into eight separated commits, so I could run isolated benchmarks against them.

Src/FluentAssertions/Equivalency/ObjectReference.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Equivalency/ObjectReference.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Equivalency/ObjectReference.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Equivalency/ObjectReference.cs Outdated Show resolved Hide resolved
{
return (@object.GetHashCode() * 397) ^ path.GetHashCode();
}
return System.Runtime.CompilerServices.RuntimeHelpers.GetHashCode(@object);
Copy link
Member

@jnyrup jnyrup Oct 5, 2018

Choose a reason for hiding this comment

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

I guess you saw that getting HashCode of the old path array, didn't make much sense.
Can you comment on why you chose RuntimeHelpers.GetHashCode over Object.GetHashCode()?

Copy link
Contributor Author

@pentp pentp Nov 9, 2018

Choose a reason for hiding this comment

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

Because we want to compare/hash the object reference not value. RuntimeHelpers.GetHashCode returns the object identity hash even if it overrides Object.GetHashCode.
I'm not sure if this is even used anywhere, it looks like ObjectReference is only used in lists.

Copy link
Member

@jnyrup jnyrup Nov 9, 2018

Choose a reason for hiding this comment

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

I tried a dig a bit in the git history to get some context:

  • ObjectReference was added in 5e6d9ee4 where it had a (to me) sensible GetHashCode.
  • It was changed in 02e71b30 from string propertyPath to string[] path, but GetHashCode was not updated.
  • probably due to that, #395 did not work as ObjectReferences are now stored in HashSet.
  • #399 reverted that PR.

@jnyrup
Copy link
Member

@jnyrup jnyrup commented Oct 10, 2018

@pentp Please let me know if I can be of any help.

If you split the PR into more isolated commits, such as e.g.

  • Replacing Expression.Lambda(),Compile() with MakeGenericMethod
  • is ICollection return early check
  • lazy splitting of path in ObjectReference
  • Replacing SequenceEqual with C-style loop in ObjectReference
  • Changing GetHashCode() in ObjectReference
  • subject != null || , expectation != null || and if (interfaceTypes.Length != 1) conditionals

I'll try to setup some benchmarks to get some numbers.

@dennisdoomen
Copy link
Member

@dennisdoomen dennisdoomen commented Nov 8, 2018

Hi @pentp, do you think you'll be able to follow up?

@pentp
Copy link
Contributor Author

@pentp pentp commented Nov 8, 2018

Yes, I planned to check up on this PR this week, so hopefully tomorrow. I have some measurements saved, just need to check which changes gave only minor improvements and probably just undo those.

@pentp pentp force-pushed the perf-fixes branch 2 times, most recently from cd214ab to 976b3df Compare Nov 12, 2018
jnyrup
jnyrup approved these changes Nov 13, 2018
@jnyrup
Copy link
Member

@jnyrup jnyrup commented Nov 13, 2018

@pentp This looks very nice!
Is this still WIP?

@pentp
Copy link
Contributor Author

@pentp pentp commented Nov 13, 2018

No, this should be ready for merge.

@jnyrup jnyrup requested a review from dennisdoomen Nov 13, 2018
@dennisdoomen
Copy link
Member

@dennisdoomen dennisdoomen commented Nov 19, 2018

Needs to be rebased

@adamvoss
Copy link
Contributor

@adamvoss adamvoss commented Nov 19, 2018

Thanks for taking a look at this part of the code base and submitting a PR.
I find it a bit difficult to review the changes, as they cover many different parts, more than just replacing the LambdaCompiler.

Can you comment more the effects the individual changes had?
E.g. you added several conditionals to avoid calls to ForCondition and I'm interested in the real world benefits of these.

What were the absolute improvements on your tests?

@adamvoss As far as I could trace this back, you authored the initial implementation of this.
Do you recall any considerations in picking LambdaCompiler approach over MakeGenericMethod?

Hi, I'm Dan Voss -- Adam's dad. I found this question while wrapping up Adam's digital accounts. I regret to inform you that Adam died in July. http://schluterbalikfuneralhome.com/obituary/adam-voss

@dennisdoomen
Copy link
Member

@dennisdoomen dennisdoomen commented Nov 19, 2018

Hi, I'm Dan Voss -- Adam's dad. I found this question while wrapping up Adam's digital accounts. I regret to inform you that Adam died in July. http://schluterbalikfuneralhome.com/obituary/adam-voss

Don't know what to say....

@pentp
Copy link
Contributor Author

@pentp pentp commented Nov 20, 2018

Rebased

@dennisdoomen dennisdoomen merged commit 014cf50 into fluentassertions:master Nov 20, 2018
1 check passed
@pentp pentp deleted the perf-fixes branch Nov 20, 2018
@adamvoss
Copy link
Contributor

@adamvoss adamvoss commented Dec 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants