improve performance and reduce allocations #40

bollhals · 2021-02-23T17:13:05Z

I've played around a bit and saw a lot of potential for improvements. The result should (😉 ) not alter public behavior but rather only improve performance / allocations:

This is the outcome for one of the Benchmarks

Master

Method	Mean	Error	StdDev	Min	Max	Median	Rank	Gen 0	Gen 1	Gen 2	Allocated
ResolveFromType.Current	243.1 ns	4.85 ns	4.30 ns	238.4 ns	252.1 ns	241.3 ns	1	0.0548	-	-	344 B
ResolveAllFromType.Current	613.3 ns	11.31 ns	10.58 ns	599.4 ns	634.4 ns	611.0 ns	1	0.1116	-	-	704 B

PR

Method	Mean	Error	StdDev	Min	Max	Median	Rank	Gen 0	Gen 1	Gen 2	Allocated
ResolveFromType.Current	168.2 ns	1.80 ns	1.50 ns	166.5 ns	171.8 ns	168.0 ns	1	-	-	-	-
ResolveAllFromType.Current	502.4 ns	9.26 ns	8.66 ns	493.1 ns	518.6 ns	499.5 ns	1	0.0343	-	-	216 B

PS: I got some weird exception when I tried to run it on .net 4.5
System.Security.VerificationException : Operation could destabilize the runtime.
so could you please check whether this is a "doesn't work on my machine"-issue :)

SabotageAndi · 2021-03-08T15:41:05Z

Sorry @bollhals for the late reply.
Thanks for the PR. It looks interesting with these numbers. Was the performance of BoDi a problem in your project? I would be interested in talking with you about it. Please get in touch with me on Discord.

But the problem for us is, that we just took over BoDi and aren't yet familiar with the codebase to a degree, were we are a little bit hesitant to merge it.
So it will probably take some time until we merge it in.

tzongithub

Hi,

unfortunately I also get the System.Security.VerificationException : Operation could destabilize the runtime. exceptions. From all the unit tests I have 9 red ones with this exception. The exception is weird but not random, it comes every time when I run the tests. Did the unit tests run green for you?

Unfortunately the PR contains a lot of changes and it is hard to review, follow the ideas, and to see if this will work at the end. It is hard to see now:

what change did break the tests (VerificationException)
what changes are improving the performance significantly
what are just nice-to-have formatting changes

The PR also changes the "public" API of ObjectContainer (sealed, removing protected virtual, etc.).

I'd ask/recommend to create smaller scoped PRs (starting with the most important improvement), keeping the changes focused and tested.

@SabotageAndi I think the single-file BoDi causes extra headache, because the diff of such bigger PRs is hardly readable. Probably we should split up the BoDi.cs file first before implementing such bigger changes in the lib.

tzongithub · 2021-04-19T14:21:56Z

BoDi/BoDi.cs

@@ -225,92 +226,93 @@ public interface IStrategyRegistration
        IStrategyRegistration InstancePerContext();
    }

-    public class ObjectContainer : IObjectContainer
+    public sealed class ObjectContainer : IObjectContainer


Why do you want to seal the public ObjectContainer class? There might be library users who need to subclass.

I've generally seal all types, but I've come to learn that this is not generally wanted in SpecFlow, will adapt.

tzongithub · 2021-04-19T14:25:24Z

BoDi/BoDi.cs

        {
            private readonly RegistrationKey currentRegistrationKey;
            private readonly Type currentResolvedType;
            private readonly ResolutionList nextNode;
-            private bool IsLast { get { return nextNode == null; } }


What's the reason behind removing the IsLast and the two different ctors?

IsLast was only used in one place, where I felt it was simpler to just use nextNode != null. (not a relevant change)

I only removed the non parameter ctor, as the usage changed to only create a ResolutionList when you need one (meaning you have at least one node)

tzongithub · 2021-04-19T14:32:58Z

BoDi/BoDi.cs

-                if (type == null) throw new ArgumentNullException("type");
+                if (type is null)
+                {
+                    ThrowNullException();


What's the reason behind this pattern? Why is the ThrowNullException local function better than the original if/throw?

Generally: Throwing a exception prevents inlining of a function.
Since I moved the typeGroup into here, it might be inlined anyway (due to size of the ctor), so there might not be a benefit in doing so anymore.

tzongithub · 2021-04-19T14:37:03Z

BoDi/BoDi.cs

-                {
-                    return TypeGroup.GetHashCode();
-                }
+                return typeGroup.GetHashCode();


Is it safe to remove the unchecked here? Why was it here originally?
Note that in the original code not only the GetHasCode call was in unchecked but the whole TypeGroup calculation. Although in the Equals the TypeGroup was called without unchecked...

I removed it as I didn't see a good reason why it was there, it gets a hashcode from a Type, so if unchecked is needed, shouldn't it be part of the Type.GetHashcode instead of here?

Also typeGroup is the cached field, TypeGroup was the property recalculating it all the time,.

tzongithub · 2021-04-19T14:51:35Z

BoDi/BoDi.cs

                var pooledObjectKey = new RegistrationKey(typeToConstruct, keyToResolve.Name);

-                var result = ExecuteWithLock(syncRoot, () => container.GetPooledObject(pooledObjectKey), () =>
+                if (container.TryGetObjectFromPool(pooledObjectKey, out var obj))


Does the copy-pasting of the ExecuteWithLock logic bring a significant performance improvement over the method call with lambdas? If yes, is there another way of optimizing this without copy-pasting the logic?

The issue with these lambdas is that they contain local state, hence they have to be allocated for every execution, slowing it down due to a) allocation, b) GC overhead in cleaning it up, c) no inlining and d) call overhead.
So yes it is quite significant / measurable.

One possible alternative would be trying to extract certain pieces into their own functions that can be called individually, and thus trying to reduce the amount of code duplicated.

tzongithub · 2021-04-19T15:09:16Z

BoDi/BoDi.cs

-            return registrations
-                .Where(x => x.Key.Type == typeof(T))
-                .Select(x => Resolve(x.Key.Type, x.Key.Name) as T);
+            foreach (var pair in this.registrations)


Is the foreach structure measurably faster than LINQ?

Yes, measurably.
In general, LINQ are nice to read but horrible in performance (especially when they do closures).
Keep in mind we're here talking in ns difference, but as this is called so often during a normal test, they all add up.

tzongithub · 2021-04-19T15:35:09Z

BoDi/BoDi.cs

            OnObjectCreated(obj);

            return obj;
        }

-        protected virtual void OnObjectCreated(object obj)
+        private void OnObjectCreated(object obj)


Protected virtual to private is a breaking change for ObjectContainer (there might be external usages)

does a derived implementation want to override this method or register themselves on ObjectCreated Event itself?

Also generic question: If someone actually wants to not use this ObjectContainer in their specflow scenario, how likely is it that one would implement their own vs derive from ObjectContainer 🤔
Also if someone would want to do it, wouldn't it be more likely to open a PR to fix something directly than implementing it in a derived class?

tzongithub · 2021-04-19T15:35:27Z

BoDi/BoDi.cs

-
-            object obj;
-            if (maxParamCountCtors.Length == 1)
+            int maxLength = -1;


In this case the new solution looks much cleaner than the LINQ magic. ;)

tzongithub · 2021-04-19T15:41:22Z

BoDi/BoDi.cs

@@ -909,22 +903,18 @@ private bool GetObjectFromPool(RegistrationKey pooledObjectKey, out object obj)

        private object ResolveObject(RegistrationKey keyToResolve, ResolutionList resolutionPath)
        {
-            if (keyToResolve.Type.IsPrimitive || keyToResolve.Type == typeof(string) || keyToResolve.Type.IsValueType)
-                throw new ObjectContainerException("Primitive types or structs cannot be resolved: " + keyToResolve.Type.FullName, resolutionPath.ToTypeList());
+            // All primitive types are structs, no need to check


Good point.

Also FYI, IsPrimitive is not an intrinsic, hence it is generally not as cheap to call as IsValueType.

tzongithub · 2021-04-19T16:34:36Z

BoDi/BoDi.cs

        private readonly ObjectContainer baseContainer;
        private readonly ConcurrentDictionary<RegistrationKey, IRegistration> registrations = new ConcurrentDictionary<RegistrationKey, IRegistration>();
-        private readonly List<RegistrationKey> resolvedKeys = new List<RegistrationKey>();
+        private readonly HashSet<RegistrationKey> resolvedKeys = new HashSet<RegistrationKey>();


I have this "too nice to be true" feeling about this: it is "too obvious" that a HashSet would be more performant instead of the List (assuming a big enough list). I wonder if there was a good (but unfortunately undocumented) reason for using a List here in the original code.

I wasn't able to find one, if there really is one, happy to check again what optimization we could apply then.

bollhals · 2021-04-19T21:59:06Z

unfortunately I also get the System.Security.VerificationException : Operation could destabilize the runtime. exceptions. From all the unit tests I have 9 red ones with this exception. The exception is weird but not random, it comes every time when I run the tests. Did the unit tests run green for you?

I have now an environment that I can reproduce it, so I might be able to find the root cause for it.

improve performance and reduce allocations

cad536b

tzongithub reviewed Apr 19, 2021

View reviewed changes

This was referenced Apr 20, 2021

improve performance of RegistrationKey #41

Open

improve allocation/performance of ResolutionList #42

Open

improve performance / allocations for registrations resolve #43

Open

improve performance / allocations on CreateObject / ResolveArgument #44

Open

This was referenced May 1, 2021

Improve performance / allocations of RegisterAs* methods #45

Open

improve ctor and dispose #46

Open

do type checks when register, not when resolving #47

Open

improve GetRegistrationResult #48

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve performance and reduce allocations #40

improve performance and reduce allocations #40

bollhals commented Feb 23, 2021

SabotageAndi commented Mar 8, 2021

tzongithub left a comment

tzongithub Apr 19, 2021

bollhals Apr 19, 2021

tzongithub Apr 19, 2021

bollhals Apr 19, 2021

tzongithub Apr 19, 2021

bollhals Apr 19, 2021

tzongithub Apr 19, 2021

bollhals Apr 19, 2021

tzongithub Apr 19, 2021

bollhals Apr 19, 2021 •

edited

Loading

tzongithub Apr 19, 2021

bollhals Apr 19, 2021

tzongithub Apr 19, 2021

bollhals Apr 19, 2021

tzongithub Apr 19, 2021

tzongithub Apr 19, 2021

bollhals Apr 19, 2021

tzongithub Apr 19, 2021 •

edited

Loading

bollhals Apr 19, 2021

bollhals commented Apr 19, 2021

improve performance and reduce allocations #40

Are you sure you want to change the base?

improve performance and reduce allocations #40

Conversation

bollhals commented Feb 23, 2021

SabotageAndi commented Mar 8, 2021

tzongithub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bollhals Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzongithub Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bollhals commented Apr 19, 2021

bollhals Apr 19, 2021 •

edited

Loading

tzongithub Apr 19, 2021 •

edited

Loading