Improve performance by more than 400X #350
Conversation
Scenarios: * IEnumerable option with 9000 entries: 15+ hours -> 150ms * Options class w/20 string options all set on command line: 960ms -> 2.2ms The speedups were all achieved by memoizing IEnumerables that are enumerated multiple times.
| @@ -28,13 +28,15 @@ static class InstanceBuilder | |||
| var typeInfo = factory.MapValueOrDefault(f => f().GetType(), typeof(T)); | |||
|
|
|||
| var specProps = typeInfo.GetSpecifications(pi => SpecificationProperty.Create( | |||
| Specification.FromProperty(pi), pi, Maybe.Nothing<object>())); | |||
| Specification.FromProperty(pi), pi, Maybe.Nothing<object>())) | |||
| .Memorize(); | |||
There was a problem hiding this comment.
I used .Memorize() throughout this PR since that seems to be the dominant existing convention.
However, it feels like an anti-pattern to me to pass IEnumerable everywhere rather than, say, IReadOnlyCollection. It makes it difficult to detect multi-enumeration issues visually or via static analysis such as Resharper. IEnumerable definitely has its place, it just feels overused.
What is the rationale behind the current approach?
There was a problem hiding this comment.
What is the rationale behind the current approach?
IReadOnlyCollection was introduced in .NET 4.5. This library still targets 4.0 on the fat framework.
That, plus the developer was really inspired by functional languages and the "lazy evaluation" that's built in to many of them. Unfortunately, there are (perf) penalties for using C# as if it were a functional language.
I initially added .ToSet() to EnumerableExtensions.cs, not realizing it was a paket file and thus not directly editable in this repo.
|
Thank you @bergbria! These changes are awesome. I'm curious if your perf tests showed much of a difference in memory usage due to the caching, but even if there's a difference I imagine it's pretty small. |
|
I haven't measured relative memory usage. While there probably is a
difference, I agree that it would be small. It should also be GC'd pretty
quickly since nothing should hang on to the objects. I can gather more
numbers tomorrow if you'd like.
How long do you think will it take to get an official Nuget package with
this change? I can't update our codebase to use v2 without it. I can always
publish my own package somewhere, but I'd prefer not to.
…On Thu, Oct 18, 2018 at 5:05 PM Dan Nemec ***@***.***> wrote:
Thank you @bergbria <https://github.com/bergbria>! These changes are
awesome. I'm curious if your perf tests showed much of a difference in
memory usage due to the caching, but even if there's a difference I imagine
it's pretty small.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#350 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACqWyfLNctiZcm5fhpxOoqh3OgXDwC7Sks5umRcxgaJpZM4XvS0_>
.
|
|
@nemec, if you get a chance today, mind commenting on my deployment question? I'll just default to creating my own package to unblock myself if I don't hear from you. |
|
@bergbria wow, thanks for addressing that! If your perf improvements are what you say, thats incredible. I'm trying to get a v3 out with updated references to prevent a couple of issues that are popping up with netcore and netframework package dependencies. We can probably issue a new v2 release to include this. (I'm a little perplexed that you can't move to v2 without it though... lol) My only real question on these improvements is whether they are using static variables (which will hang around the entire process lifetime) and using a ctor reference that can be GC'd. If its the latter, perfect! (Haven't had a chance to peruse the PR yet) |
|
@ericnewton76 I didn't introduce anything static. My only changes were to local variable definitions, so persistent memory usage shouldn't be an issue. Our code is currently using v1 with some very large rsp files (10k+ arguments passed inside a file). It's performant enough to not be an issue. We can't really move to v2 if arg parsing starts taking 15+ hours. Removing the rsps on our end would be difficult and less desirable than taking the perf fix. |
|
@ericnewton76 any chance of getting a new release out today or early next week? |
Scenarios:
The speedups were predominantly achieved by memoizing IEnumerables that are enumerated multiple times. A comparatively minor speedup was achieved by using a HashSet rather than LINQ's .Contains().
There should be no behavioral differences introduced by this change.
It should resolve #186.