Replace Mock.Of code with 10x faster equivalent #598
Conversation
|
Fixing ... |
|
The time required to create a mock depends on the complexity of the type you want to mock. So including one single benchmark for one single mocked type wouldn't really give the full picture. All I wanted to see with that benchmark is an approximate speed difference (order of magnitude). I'll review your PR a little later! |
CHANGELOG.md
Outdated
| @@ -11,6 +11,9 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1 | |||
|
|
|||
| * Add `ISetupSequentialResult<TResult>.Returns` method overload that support delegate for deferred results (@snrnats, #594) | |||
|
|
|||
| #### Changed | |||
|
|
|||
| * Changed `Mock.Of<T>()` implementation to use much faster simple approach (@informatorius, #547) | |||
There was a problem hiding this comment.
In principle, this is a perfectly fine changelog entry. But may I humbly suggest that you shorten & rephrase it to be even more relevant from the user's perspective? For example:
- Speed up
Mock.Of<T>()by approx. one order of magnitude (...)
There was a problem hiding this comment.
Oh btw., please replace #547 (issue) with #598 (this PR).
Source/Linq/Mock.cs
Outdated
| return Mocks.CreateMockQuery<T>().First<T>(); | ||
| //Disabled because of bad performance | ||
| //return Mocks.CreateMockQuery<T>().First<T>(); | ||
| //and replaced by equivalent which is more than 10 times faster currently |
There was a problem hiding this comment.
Hmm, IMHO using the word "bad" isn't quite fair. I totally agree that the current implementation has worse performance than your new one; but does that make it bad?
I would've chosen a more neutral wording that avoids negative judgement; for example:
// This method was originally implemented as follows:
//
// return Mocks.CreateMockQuery<T>().First<T>();
//
// which involved a lot of avoidable `IQueryable` query provider overhead and
// lambda compilation. What it really boils down to is this (much faster) code:
...
CHANGELOG.md
Outdated
| @@ -11,6 +11,9 @@ The format is loosely based on [Keep a Changelog](http://keepachangelog.com/en/1 | |||
|
|
|||
| * Add `ISetupSequentialResult<TResult>.Returns` method overload that support delegate for deferred results (@snrnats, #594) | |||
|
|
|||
| #### Changed | |||
|
|
|||
| * Changed `Mock.Of<T>()` implementation to use much faster simple approach (@informatorius, #547) | |||
There was a problem hiding this comment.
Please add one more blank line after this one.
|
Done! Thanks for your perfect feedback. |
|
Thank you for contributing this much faster reimplementation of |
|
@informatorius - I just pushed Moq 4.8.3 to NuGet, which includes your changes. Sorry it's taken so long. |
#547
PR not finished yet ...