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

Regression in .net 4.7.1 #500

Closed
StasPerekrestov opened this issue Oct 23, 2017 · 4 comments
Closed

Regression in .net 4.7.1 #500

StasPerekrestov opened this issue Oct 23, 2017 · 4 comments

Comments

@StasPerekrestov
Copy link

Hello,

just FYI. There is a potential regression in .net 4.7.1 that significantly affects moq Setup API logic.

@stakx
Copy link
Contributor

stakx commented Oct 23, 2017

@StasPerekrestov - thanks for linking that issue and letting us know!

Regarding Moq's use of StackTrace in the code location you linked to (MethodCall.GetFileInfo): it's a nice to have debug feature, but nonessential, and I feel that it might eventually have to be dropped anyway—it's not supported on .NET Core, and it's already a noticeable performance bottleneck on the .NET Framework (IIRC; I checked a few months ago with PerfView, but don't remember the exact details).

Let's leave this issue open so we can gather some benchmark data on just how badly Moq's performance is affected—by its use of StackTrace in general, and under .NET 4.7.1 specifically—and then figure out if we should do something about it.

@Caleb9
Copy link

Caleb9 commented Nov 6, 2017

@stakx I don't have access to machine without latest Windows 10 Creators update, so I can't really give you comparison times, but this issue hit me really hard. For example I have some tests that setup things on Mock<MethodInfo> (need to test something that depends on Castle.Core's IInvocation type). Setting up a single get property takes about 1500 ms right now with pure Moq. Since I am using AutoFixture.AutoMoq package, there is a lot of setup happening behind the scenes. This combination (IInvocation, MethodInfo and AutoFixture.AutoMoq) makes some of my test to take 15 seconds to execute now, while they were perfectly fine and fast unit tests before the update... Not sure how likely it is for Microsoft to fix it on their side, but I'd like to give my vote for removing that feature if you say it's nonessential. :)

@stakx
Copy link
Contributor

stakx commented Nov 6, 2017

@Caleb9: Your vote has been heard. :)

I would say it is very likely that Microsoft will fix this. However, I realise that until this happens, the situations is... problematic.

Moq currently collects source file information for each setup by default. I think this needs to become an opt-in feature: it's there if people want this diagnostic information; and it won't incur any performance cost for people who don't need it.

Given that version 4.8.0 of Moq isn't quite ready yet I'm wondering whether a hotfix release is required...?

@stakx
Copy link
Contributor

stakx commented Nov 6, 2017

@Caleb9, @StasPerekrestov - I have just published a hotfix release of Moq (version 4.7.145). Source file information collection via StackTrace is now disabled by default, meaning the .NET Framework regression should no longer degrade test execution speed.

If you want the old behavior back, i.e. you want better diagnostic messages when doing a mockRepository.Verify[All], you need to opt back in via mock[Repository].Switches |= Switches.CollectDiagnosticFileInfoForSetups.

You shouldn't have any more trouble regarding this issue after you upgrade to 4.7.145 or later. If you do, please post back here!

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

No branches or pull requests

3 participants