Skip to content

Resolved a significant performance degradation in BeEquivalentTo#1660

Merged
dennisdoomen merged 1 commit intofluentassertions:masterfrom
dennisdoomen:Fix/CallerIdentifierPerformance
Aug 22, 2021
Merged

Resolved a significant performance degradation in BeEquivalentTo#1660
dennisdoomen merged 1 commit intofluentassertions:masterfrom
dennisdoomen:Fix/CallerIdentifierPerformance

Conversation

@dennisdoomen
Copy link
Copy Markdown
Member

@dennisdoomen dennisdoomen commented Aug 22, 2021

This was caused by the increased dependency on the CallerIdentifier, and the additional work caller identification takes in v6.

#1657

@dennisdoomen dennisdoomen force-pushed the Fix/CallerIdentifierPerformance branch from dc2fb5b to e008609 Compare August 22, 2021 12:27
@dennisdoomen dennisdoomen requested a review from jnyrup August 22, 2021 12:35
jnyrup
jnyrup approved these changes Aug 22, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@jnyrup jnyrup left a comment

Choose a reason for hiding this comment

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

Benchmark results:

master:

|              Runtime |  N |            Mean |         Error |        StdDev |       Gen 0 |     Gen 1 |    Allocated |
|--------------------- |--- |----------------:|--------------:|--------------:|------------:|----------:|-------------:|
|             .NET 5.0 |  1 |        57.09 us |      0.299 us |      0.279 us |     11.1694 |    0.3052 |        69 KB |
| .NET Framework 4.7.2 |  1 |     2,025.39 us |     10.364 us |      9.695 us |    136.7188 |    7.8125 |       855 KB |
|             .NET 5.0 | 10 |   165,775.29 us |  1,407.268 us |  1,316.360 us |  11000.0000 | 1000.0000 |    68,264 KB |
| .NET Framework 4.7.2 | 10 |   216,483.12 us |    687.123 us |    573.779 us |  14333.3333 | 1000.0000 |    88,899 KB |
|             .NET 5.0 | 50 | 4,604,075.87 us | 28,517.080 us | 26,674.895 us | 308000.0000 | 1000.0000 | 1,892,259 KB |
| .NET Framework 4.7.2 | 50 | 5,552,894.02 us | 47,078.673 us | 41,734.018 us | 365000.0000 | 1000.0000 | 2,243,784 KB |

PR 1660

|              Runtime |  N |          Mean |        Error |       StdDev |      Gen 0 |     Gen 1 |  Allocated |
|--------------------- |--- |--------------:|-------------:|-------------:|-----------:|----------:|-----------:|
|             .NET 5.0 |  1 |      56.26 us |     0.351 us |     0.328 us |    11.1694 |    0.3052 |      69 KB |
| .NET Framework 4.7.2 |  1 |     277.59 us |     3.679 us |     3.441 us |    28.8086 |    0.9766 |     178 KB |
|             .NET 5.0 | 10 |  11,297.99 us |    25.180 us |    23.553 us |  1468.7500 |  140.6250 |   9,038 KB |
| .NET Framework 4.7.2 | 10 |  16,182.11 us |   320.831 us |   315.099 us |  1843.7500 |  156.2500 |  11,492 KB |
|             .NET 5.0 | 50 | 329,054.39 us | 1,672.983 us | 1,483.056 us | 40000.0000 | 7000.0000 | 249,728 KB |
| .NET Framework 4.7.2 | 50 | 462,394.07 us | 1,273.735 us | 1,129.133 us | 50000.0000 | 2000.0000 | 308,181 KB |

Tests/FluentAssertions.Specs/Equivalency/CollectionEquivalencySpecs.cs
}

[Fact]
public void Comparing_lots_of_complex_objects_should_still_be_fast()
Copy link
Copy Markdown
Member

@jnyrup jnyrup Aug 22, 2021

Choose a reason for hiding this comment

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

Failed in pipeline

Copy link
Copy Markdown
Member Author

@dennisdoomen dennisdoomen Aug 22, 2021

Choose a reason for hiding this comment

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

Yeah, AppVeyor is a lot slower. Had to increase the threshold to 20 seconds.

@dennisdoomen
Resolved a significant performance degradation in BeEquivalentTo
c3d260a 
This was caused by the increased dependency on the CallerIdentifier, and the additional work caller identification takes in v6.
@dennisdoomen dennisdoomen force-pushed the Fix/CallerIdentifierPerformance branch from e008609 to c3d260a Compare August 22, 2021 13:49
@dennisdoomen
Copy link
Copy Markdown
Member Author

dennisdoomen commented Aug 22, 2021

All in all, I think it would be nice if we can come up with some approach to fail the build if those benchmarks exceed a certain value.

@dennisdoomen dennisdoomen merged commit 577d555 into fluentassertions:master Aug 22, 2021
@dennisdoomen dennisdoomen deleted the Fix/CallerIdentifierPerformance branch August 22, 2021 15:36
@jnyrup jnyrup mentioned this pull request Aug 22, 2021
Merged
@lg2de
Copy link
Copy Markdown
Contributor

lg2de commented Sep 1, 2021

@jnyrup, you created a benchmark but did not added it to the solution?
Or was THIS benchmark already existing?

@jnyrup
Copy link
Copy Markdown
Member

jnyrup commented Sep 1, 2021

@lg2de I used the example in #1657 for my benchmark, but didn't add it.
Good point about adding it - I'll do that.

@jnyrup
Copy link
Copy Markdown
Member

jnyrup commented Sep 5, 2021

I was just about to add the benchmarks, but seems my former self already did that in #1661.

mdentremont
mdentremont reviewed Oct 1, 2021
View reviewed changes
Copy link
Copy Markdown

@mdentremont mdentremont left a comment

Choose a reason for hiding this comment

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

Good catch and fix!

Elanis pushed a commit to Elanis/portfolio that referenced this pull request Dec 13, 2022
Bump FluentAssertions from 5.10.3 to 6.1.0 (#8)
ef87c5c 
Bumps [FluentAssertions](https://github.com/fluentassertions/fluentassertions) from 5.10.3 to 6.1.0.
<details>
<summary>Release notes</summary>

*Sourced from [FluentAssertions's releases](https://github.com/fluentassertions/fluentassertions/releases).*

> ## 6.1.0
> [Release Notes](https://fluentassertions.com/releases/[#610](https://github.com/fluentassertions/fluentassertions/issues/610))
>
> ## 6.0.0
> [Release Notes](https://github.com/fluentassertions/fluentassertions/blob/master/docs/_pages/releases.md#600)
>
> ## 6.0.0-beta.3
> [Release Notes](https://github.com/fluentassertions/fluentassertions/blob/6.0.0-beta.3/docs/_pages/releases.md#600-beta-3)
>
> ## 6.0.0-beta.2
> [Release Notes](https://github.com/fluentassertions/fluentassertions/blob/release-6.0/docs/_pages/releases.md#600-beta-2)
>
> ## 6.0.0-beta.1
> [Release Notes](https://github.com/fluentassertions/fluentassertions/blob/release-6.0/docs/_pages/releases.md#600-beta-1)
>
> ## 6.0.0-alpha.2
> See [release notes](https://github.com/fluentassertions/fluentassertions/blob/develop/docs/_pages/releases.md#600-alpha-2).
>
> ## 6.0.0-alpha.1
> See https://github.com/fluentassertions/fluentassertions/blob/develop/docs/_pages/releases.md#600
</details>
<details>
<summary>Commits</summary>

- [`ce07e5c`](fluentassertions/fluentassertions@ce07e5c) Bump the release number for 6.1
- [`577d555`](fluentassertions/fluentassertions@577d555) Resolved a significant performance degradation in `BeEquivalentTo` ([#1660](fluentassertions/fluentassertions#1660))
- [`a38a54a`](fluentassertions/fluentassertions@a38a54a) Fix small grammatical error ([#1659](fluentassertions/fluentassertions#1659))
- [`8ad5e52`](fluentassertions/fluentassertions@8ad5e52) Merge pull request [#1658](fluentassertions/fluentassertions#1658) from LennartKoot/async-ex-asserts
- [`559e10b`](fluentassertions/fluentassertions@559e10b) Update release page
- [`f69c87c`](fluentassertions/fluentassertions@f69c87c) Update approved api
- [`1d6b2a3`](fluentassertions/fluentassertions@1d6b2a3) Add extensions method 'WithInnerExceptionExactly()' for async ExceptionAssert...
- [`a977427`](fluentassertions/fluentassertions@a977427) Merge pull request [#1656](fluentassertions/fluentassertions#1656) from eNeRGy164/markdown-linting
- [`d6ea6c1`](fluentassertions/fluentassertions@d6ea6c1) Update header style and add additional installation path
- [`75bfd96`](fluentassertions/fluentassertions@75bfd96) Correct white-space in documentation
- Additional commits viewable in [compare view](fluentassertions/fluentassertions@5.10.3...6.1.0)
</details>

<br />

Reviewed-on: https://gitea.dysnomia.studio/elanis/portfolio/pulls/8
Co-authored-by: elanis <[email protected]>
Co-committed-by: elanis <[email protected]>
Elanis added a commit to Dysnomia-Studio/dehash-me that referenced this pull request Mar 12, 2023
@Elanis
Bump FluentAssertions from 5.10.3 to 6.1.0 (#14)
ed3c90d 
Bumps [FluentAssertions](https://github.com/fluentassertions/fluentassertions) from 5.10.3 to 6.1.0.
<details>
<summary>Release notes</summary>

*Sourced from [FluentAssertions's releases](https://github.com/fluentassertions/fluentassertions/releases).*

> ## 6.1.0
> [Release Notes](https://fluentassertions.com/releases/[#610](https://github.com/fluentassertions/fluentassertions/issues/610))
>
> ## 6.0.0
> [Release Notes](https://github.com/fluentassertions/fluentassertions/blob/master/docs/_pages/releases.md#600)
>
> ## 6.0.0-beta.3
> [Release Notes](https://github.com/fluentassertions/fluentassertions/blob/6.0.0-beta.3/docs/_pages/releases.md#600-beta-3)
>
> ## 6.0.0-beta.2
> [Release Notes](https://github.com/fluentassertions/fluentassertions/blob/release-6.0/docs/_pages/releases.md#600-beta-2)
>
> ## 6.0.0-beta.1
> [Release Notes](https://github.com/fluentassertions/fluentassertions/blob/release-6.0/docs/_pages/releases.md#600-beta-1)
>
> ## 6.0.0-alpha.2
> See [release notes](https://github.com/fluentassertions/fluentassertions/blob/develop/docs/_pages/releases.md#600-alpha-2).
>
> ## 6.0.0-alpha.1
> See https://github.com/fluentassertions/fluentassertions/blob/develop/docs/_pages/releases.md#600
</details>
<details>
<summary>Commits</summary>

- [`ce07e5c`](fluentassertions/fluentassertions@ce07e5c) Bump the release number for 6.1
- [`577d555`](fluentassertions/fluentassertions@577d555) Resolved a significant performance degradation in `BeEquivalentTo` ([#1660](fluentassertions/fluentassertions#1660))
- [`a38a54a`](fluentassertions/fluentassertions@a38a54a) Fix small grammatical error ([#1659](fluentassertions/fluentassertions#1659))
- [`8ad5e52`](fluentassertions/fluentassertions@8ad5e52) Merge pull request [#1658](fluentassertions/fluentassertions#1658) from LennartKoot/async-ex-asserts
- [`559e10b`](fluentassertions/fluentassertions@559e10b) Update release page
- [`f69c87c`](fluentassertions/fluentassertions@f69c87c) Update approved api
- [`1d6b2a3`](fluentassertions/fluentassertions@1d6b2a3) Add extensions method 'WithInnerExceptionExactly()' for async ExceptionAssert...
- [`a977427`](fluentassertions/fluentassertions@a977427) Merge pull request [#1656](fluentassertions/fluentassertions#1656) from eNeRGy164/markdown-linting
- [`d6ea6c1`](fluentassertions/fluentassertions@d6ea6c1) Update header style and add additional installation path
- [`75bfd96`](fluentassertions/fluentassertions@75bfd96) Correct white-space in documentation
- Additional commits viewable in [compare view](fluentassertions/fluentassertions@5.10.3...6.1.0)
</details>

<br />

Co-authored-by: Elanis <[email protected]>
Reviewed-on: https://gitea.dysnomia.studio/elanis/dehash-me/pulls/14
Co-authored-by: elanis <[email protected]>
Co-committed-by: elanis <[email protected]>
Elanis added a commit to Dysnomia-Studio/dysnomia-website that referenced this pull request Jul 14, 2023
@Elanis
Bump FluentAssertions from 5.10.3 to 6.1.0 (#11)
dd0b640 
Bumps [FluentAssertions](https://github.com/fluentassertions/fluentassertions) from 5.10.3 to 6.1.0.
<details>
<summary>Release notes</summary>

*Sourced from [FluentAssertions's releases](https://github.com/fluentassertions/fluentassertions/releases).*

> ## 6.1.0
> [Release Notes](https://fluentassertions.com/releases/[#610](https://github.com/fluentassertions/fluentassertions/issues/610))
>
> ## 6.0.0
> [Release Notes](https://github.com/fluentassertions/fluentassertions/blob/master/docs/_pages/releases.md#600)
>
> ## 6.0.0-beta.3
> [Release Notes](https://github.com/fluentassertions/fluentassertions/blob/6.0.0-beta.3/docs/_pages/releases.md#600-beta-3)
>
> ## 6.0.0-beta.2
> [Release Notes](https://github.com/fluentassertions/fluentassertions/blob/release-6.0/docs/_pages/releases.md#600-beta-2)
>
> ## 6.0.0-beta.1
> [Release Notes](https://github.com/fluentassertions/fluentassertions/blob/release-6.0/docs/_pages/releases.md#600-beta-1)
>
> ## 6.0.0-alpha.2
> See [release notes](https://github.com/fluentassertions/fluentassertions/blob/develop/docs/_pages/releases.md#600-alpha-2).
>
> ## 6.0.0-alpha.1
> See https://github.com/fluentassertions/fluentassertions/blob/develop/docs/_pages/releases.md#600
</details>
<details>
<summary>Commits</summary>

- [`ce07e5c`](fluentassertions/fluentassertions@ce07e5c) Bump the release number for 6.1
- [`577d555`](fluentassertions/fluentassertions@577d555) Resolved a significant performance degradation in `BeEquivalentTo` ([#1660](fluentassertions/fluentassertions#1660))
- [`a38a54a`](fluentassertions/fluentassertions@a38a54a) Fix small grammatical error ([#1659](fluentassertions/fluentassertions#1659))
- [`8ad5e52`](fluentassertions/fluentassertions@8ad5e52) Merge pull request [#1658](fluentassertions/fluentassertions#1658) from LennartKoot/async-ex-asserts
- [`559e10b`](fluentassertions/fluentassertions@559e10b) Update release page
- [`f69c87c`](fluentassertions/fluentassertions@f69c87c) Update approved api
- [`1d6b2a3`](fluentassertions/fluentassertions@1d6b2a3) Add extensions method 'WithInnerExceptionExactly()' for async ExceptionAssert...
- [`a977427`](fluentassertions/fluentassertions@a977427) Merge pull request [#1656](fluentassertions/fluentassertions#1656) from eNeRGy164/markdown-linting
- [`d6ea6c1`](fluentassertions/fluentassertions@d6ea6c1) Update header style and add additional installation path
- [`75bfd96`](fluentassertions/fluentassertions@75bfd96) Correct white-space in documentation
- Additional commits viewable in [compare view](fluentassertions/fluentassertions@5.10.3...6.1.0)
</details>

<br />

Co-authored-by: Elanis <[email protected]>
Reviewed-on: https://gitea.dysnomia.studio/elanis/dysnomia-website/pulls/11
Co-authored-by: elanis <[email protected]>
Co-committed-by: elanis <[email protected]>
@jnyrup jnyrup mentioned this pull request Mar 24, 2024
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnyrup jnyrup jnyrup approved these changes

+1 more reviewer

@mdentremont mdentremont mdentremont left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dennisdoomen @lg2de @jnyrup @mdentremont