Improve NSubstitute performance #536
Conversation
zvirja
force-pushed
the
improve-performance
branch
from
cbedefa
to
d925a79
Compare
|
Thanks a lot for this @zvirja! I will try to work through this on the weekend.
I can definitely appreciate this. I think this is a sign of an excellent craftsperson! They take care of the details that others won't always notice from the outside, but are still an important part of a job well done. 👍 😄 |
| using System.Reflection; | ||
|
|
||
| namespace NSubstitute.Core | ||
| { | ||
| public class DefaultForType : IDefaultForType | ||
| { | ||
| private static readonly Dictionary<Type, object> DefaultPrimitiveValues = new Dictionary<Type, object> |
There was a problem hiding this comment.
Would it be faster to inline this as a big switch/if/else if in DefaultInstanceOfValueType?
e.g.
if (returnType == typeof(bool)) return false;
else if (returnType == typeof(char)) return '\0\;
else if /* ... */
else Activator.CreateInstance(returnType);There was a problem hiding this comment.
Thanks for the feedback. Actually, I was also in doubt regarding this 🤔
I analyzed a few big repositories and found that not all the built-in value types are equally popular. The most popular ones appeared to be bool, int and long and double. Therefore, I left optimization only for those cases and decided to handle all the rest types as it was before. That's a reasonable trade-off IMO.
P.S. I benchmarked dictionary lookup with 3 elements vs if/else check for the worse case (3rd if is successful) and the latter appeared to be 4 times faster. So you were right! 🏅
| @@ -46,7 +46,7 @@ private static void If(ICall call, Func<ICall, Predicate<EventInfo>> meetsThisSp | |||
| var matchingEvent = GetEvents(call, meetsThisSpecification).FirstOrDefault(); | |||
| if (matchingEvent != null) | |||
| { | |||
| takeThisAction(matchingEvent.Name, call.GetArguments()[0]); | |||
| takeThisAction(matchingEvent.Name, call.GetOriginalArguments()[0]); | |||
There was a problem hiding this comment.
Maybe add this commit comment to the code too? (see it doesn't get changed back in a later commit?)
There was a problem hiding this comment.
Good point! Did it for all the non-obvious places around the PR.
Presume that most of times `Rules` is empty and that rule configuration happens rarely, so it's OK to use stack (which allocates on each push).
This object is constructed on each invocation, so we optimize it to allocate less in normal case.
We are supposed to use `call.Arguments()` only if we assume that arguments might be already mutated. In this case it's not possible, as only by-ref args might be changed.
It appears that CallResults is not populated that often and most of time it's just inspected. Current implementation tries to gain from that pattern.
zvirja
force-pushed
the
improve-performance
branch
from
d925a79
to
417a08e
Compare
I've scanned a few repositories and found the most popular value types. Optimize path for those cases.
Previous approach leaded to large amount of allocations and worse performance. New approach allows to make less allocations.
zvirja
force-pushed
the
improve-performance
branch
from
417a08e
to
ac2eedc
Compare
|
@dtchepak Followed up on your concerns. Given that I fully resolved all of them, decided to proceed with a merge, so I can start working on the PR nearby. Hope you are fine. |
|
@zvirja Of course, no need to re-review for a case like this. :) |
I've reviewed the mocking container performance comparison and found that unacceptable that Moq and FakeItEasy beat us with such a big distance 😟 Therefore I spent a couple of weeks and tried to make NSubstitute a bit faster. Moq is still faster, but we are not that much behind now.
I'm happy it was possible to achieve boost with relatively small amount of internal changes. Also notice, that I drastically decreased amount of memory allocations.
I extended our benchmarks to have more and cover basic usage scenarios. Here you can find the performance difference (took me a 3-4 hours to actually run all the benchmarks twice 😮).
See benchmark results
I advice you to read changes commit by commit, so you would understand which exact optimizations I applied.
Happy review 🍷
@alexandrnikitin You might be interested as well.
P.S. I'm sane and fully admit that it makes negligible difference in real life, but I still like to see and realize that our tool is improved and fast 😊