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

Fix for slow down with many setups on single mock #1111

Merged
merged 5 commits into from
Dec 28, 2020
Merged

Fix for slow down with many setups on single mock #1111

merged 5 commits into from
Dec 28, 2020

Conversation

CeesKaas
Copy link
Contributor

@CeesKaas CeesKaas commented Nov 28, 2020

I've played with the code a bit this morning and this change fixes the unittests I've added (at the cost of a bit of extra memory consumption and an extra operation always to save n operations in most cases)

fixes #1110

@CeesKaas CeesKaas changed the title Attempted fix for #1110 Attempted fixes #1110 Nov 28, 2020
@CeesKaas CeesKaas changed the title Attempted fixes #1110 Attempted fix for slow down with many setups on single mock Nov 28, 2020
Copy link
Contributor

@stakx stakx left a comment

Choose a reason for hiding this comment

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

Hi @CeesKaas, thanks for your patience.

I do agree that we should do something about this problem. I've thought about it for a bit, and your solution indeed seems to be the easiest.

Just a couple of details that need your attention. Then this should be good to merge!

tests/Moq.Tests/MockFixture.cs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@stakx stakx added this to the 4.15.3 milestone Dec 28, 2020
CeesKaas and others added 2 commits December 28, 2020 22:05
Co-authored-by: stakx <stakx@eml.cc>
@CeesKaas CeesKaas requested a review from stakx December 28, 2020 21:13
@@ -101,6 +105,7 @@ public void Clear()
lock (this.setups)
{
this.setups.Clear();
this.activeInvocationShapes.Clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a remark for posteriority: in theory, we should also filter out removed setups in RemoveAllPropertyAccessorSetups. Not doing means that the first time a property setup is recreated after a call to mock.SetupAllProperties(), there may be a superfluous call to MarkOverriddenSetups. This is an edge case however, and the overhead of the additional MarkOverriddenSetups in this case is probably quite small, so let's just ignore this for now.

@stakx stakx changed the title Attempted fix for slow down with many setups on single mock Fix for slow down with many setups on single mock Dec 28, 2020
@stakx stakx merged commit 08d684c into devlooped:master Dec 28, 2020
@stakx
Copy link
Contributor

stakx commented Dec 28, 2020

All good. Thanks for contributing, @CeesKaas! 🚀

@stakx stakx modified the milestones: 4.15.3, 4.16.0 Jan 1, 2021
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.

Adding a setup looks like it became an O(n*n) operation since #984 was merged
2 participants