Memory traffic optimization #919

Belorus · 2018-05-30T09:07:15Z

Hi!

I work on mobile games written in C#/Xamarin, and start time is a bit critical there.
To improve it a bit, i slightly optimized Autofac memory allocations during registration phase to get rid of some simple ones:

foreach over interface (boxing of struct iterator)
allocation of iterators inside LINQ calls
allocation of delegates for LINQ calls

Also i introduced a bit more fine-grained control of default services.

In my codebase with 1600 registrations results are following:
4.8.1: 122K allocations (3.45MB), 80K collected (2.2MB)
4.8.1+PR: 82K allocations (2.25MB), 46K collected (1.20MB)

Speedup was about ~10% on .NET 4.6 (No sample, made 10 runs with logging of Stopwatch and compared best timings).

We use this fork for over a year, and i though someone else could find it useful.

TO BE UPDATED: I'll add some runtime optimizations (Resolve-time)

Replaced some Collection interfaces with specific type declarations up to 30% reduction of memory allocation traffic during registration

tillig · 2018-05-30T15:14:02Z

Thanks for the PR! There is a lot to unpack here so it will take some time. I don't think we'll be able to pull the whole thing as-is because in some cases it changes the public API and will cause consumers to break. We will likely have to find a balance of what we can optimize vs. what we can't based on how breaking it will be. There are definitely a few free ones we can easily take.

alexmg · 2018-06-05T12:37:23Z

Thanks for the PR @Belorus. There are some great optimizations here. I agree with @tillig that we need to figure out what can come straight in and what might have to wait for a 5.0 release when breaking changes are to be expected. There are a few places that are technically breaking changes because a class is public but the likelihood of someone using it directly are very low.

I've wondered for a while now how much time would be saved at resolve time (particularly the first call that initializes the ServiceRegistrationInfo) by removing IRegistrationSources that are not required.

We have a benchmarks project that might be a good way to quantify just how much benefit some of the changes bring. In the past BenchmarkDotNet had the MemoryDiagnoser enabled by default but that was changed in a later version so the GC and memory allocation information isn't currently available but could be added back easily.

I will try to take a first pass over the review and see if things can be flagged for immediate inclusion. I'm very excited to see a PR that is focused on performance.

alexmg · 2018-06-05T12:39:04Z

I just noticed that the PR targets master directly as well. They would need to come into the develop branch first. Let's wait and see how the slicing goes before doing anything though.

Belorus · 2018-06-05T13:13:13Z

Thanks for the review guys!
Then let me split this PR into 2 parts. One nonbreaking and second breaking. And I'll rebase it on develop.

alexmg · 2018-06-05T13:17:28Z

I'm doing a quick pass through now and adding a few comments that might help. There are a few changes that are technically breaking that I would probably be fine with.

alexmg

Looks great overall. Just have to make some decisions on what "technically" breaking changes we are cool with.

alexmg · 2018-06-05T13:08:37Z

src/Autofac/Builder/ContainerBuildOptions.cs

-        /// Prevents inclusion of standard modules like support for
-        /// relationship types including <see cref="IEnumerable{T}"/> etc.
-        /// </summary>
-        ExcludeDefaultModules = 2,


This is a breaking change. We could add an overload to Build takes the DefaultServiceFlags and passes through None.

Agreed, an overload would be better.

alexmg · 2018-06-05T13:10:11Z

src/Autofac/Builder/IRegistrationBuilder.cs

+        /// </summary>
+        /// <param name="service">Service types to expose.</param>
+        /// <returns>A registration builder allowing further configuration of the component.</returns>
+        IRegistrationBuilder<TLimit, TActivatorData, TRegistrationStyle> As(Type service);


Looks like these are to prevent the need to call the params method with the array allocation.

Adding this is technically a breaking change. Super low risk, but breaking. Curious if adding this would be a problem during an upgrade where custom extension methods (that people have written) indirectly call this and the compiler will get confused as to which method it should be calling. We had some problems before adding optional parameters to an existing public API method where it was seen as breaking in odd and unfortunate ways.

alexmg · 2018-06-05T13:13:19Z

src/Autofac/Builder/ReflectionActivatorData.cs

@@ -108,11 +108,11 @@ public IConstructorSelector ConstructorSelector
        /// <summary>
        /// Gets the explicitly bound constructor parameters.
        /// </summary>
-        public IList<Parameter> ConfiguredParameters { get; } = new List<Parameter>();
+        public List<Parameter> ConfiguredParameters { get; } = new List<Parameter>();


I don't think this would cause any issues from the API perspective.

Agreed, likely not a big deal. Unclear how much this stuff gets used, if ever, outside of here.

I'm trying to put this together with the change from Autofac.Core.Activators.Reflection.ReflectionActivator. It looks like the change here wants to expose these as List<T>. These appear to be primarily used when instantiating a ReflectionActivator. The change in ReflectionActivator is to stop taking an IEnumerable<T> and take an IList<T> instead.

Could we leave this alone (as IList<T>) and only make the change in ReflectionActivator? As it stands there's still going to be some implicit casting going on, which I'm guessing is the point of this.

alexmg · 2018-06-05T13:15:41Z

src/Autofac/Builder/RegistrationBuilder.cs

@@ -149,7 +149,7 @@ public static class RegistrationBuilder
            Guid id,
            RegistrationData data,
            IInstanceActivator activator,
-            IEnumerable<Service> services)
+            Service[] services)


These are definitely breaking changes but I don't think these methods would be used directly by consumers very often.

Agreed. Pretty low risk.

I brought all the changes from this file into the develop branch.

alexmg · 2018-06-05T13:17:49Z

src/Autofac/Builder/RegistrationBuilder.cs

@@ -179,11 +179,15 @@ public static class RegistrationBuilder
            var limitType = activator.LimitType;
            if (limitType != typeof(object))
            {
-                foreach (var ts in services.OfType<IServiceWithType>())
+                foreach (var ts in services)


alexmg · 2018-06-05T13:26:53Z

src/Autofac/Core/Registration/ComponentRegistration.cs

@@ -56,20 +56,21 @@ public class ComponentRegistration : Disposable, IComponentRegistration
            IComponentLifetime lifetime,
            InstanceSharing sharing,
            InstanceOwnership ownership,
-            IEnumerable<Service> services,
+            Service[] services,


Breaking change but also probably not something called directly too often.

alexmg · 2018-06-05T13:27:33Z

src/Autofac/Core/Registration/ComponentRegistry.cs

-                .Where(si => si.Value.ShouldRecalculateAdaptersOn(registration))
-                .Select(si => si.Key)
-                .ToArray();
+            List<Service> adapterServices = new List<Service>();


alexmg · 2018-06-05T13:28:31Z

src/Autofac/Core/Registration/ServiceRegistrationInfo.cs

-                    .Concat(_sourceImplementations)
-                    .Concat(_preserveDefaultImplementations);
+
+                var resultingCollection = Enumerable.Reverse(_defaultImplementations);


Everything in this file looks good.

I brought all the changes from this file into the develop branch.

alexmg · 2018-06-05T13:29:11Z

src/Autofac/Core/Resolving/CircularDependencyDetector.cs

@@ -61,8 +61,11 @@ public static void CheckForCircularDependency(IComponentRegistration registratio
                throw new DependencyResolutionException(string.Format(CultureInfo.CurrentCulture, CircularDependencyDetectorResources.MaxDepthExceeded, registration));

            // Checks for circular dependency
-            if (activationStack.Any(a => a.ComponentRegistration == registration))
-                throw new DependencyResolutionException(string.Format(CultureInfo.CurrentCulture, CircularDependencyDetectorResources.CircularDependency, CreateDependencyGraphTo(registration, activationStack)));
+            foreach (var a in activationStack)


I brought all the changes from this file into the develop branch.

alexmg · 2018-06-05T13:29:30Z

src/Autofac/Features/OpenGenerics/OpenGenericDecoratorRegistrationSource.cs

        {
            var parameter = new ResolvedParameter(
                (pi, c) => pi.ParameterType == decoratedParameterType,
                (pi, c) => c.ResolveComponent(decoratedComponent, Enumerable.Empty<Parameter>()));

-            return new[] { parameter }.Concat(configuredParameters);
+            var resultArray = new Parameter[configuredParameters.Count + 1];


I brought all the changes from this file into the develop branch.

alexmg · 2018-06-05T13:32:38Z

I've added some comments in a review @tillig. Would be interested to see if you agree on some of the changes that are technically breaking but very unlikely to have been used directly.

tillig · 2018-06-05T15:18:50Z

src/Autofac/Autofac.csproj

@@ -4,7 +4,7 @@
    <Description>Autofac is an IoC container for Microsoft .NET. It manages the dependencies between classes so that applications stay easy to change as they grow in size and complexity.</Description>
    <VersionPrefix>4.8.1</VersionPrefix>
    <TargetFrameworks>netstandard1.1;net45</TargetFrameworks>
-    <NoWarn>$(NoWarn);CS1591</NoWarn>
+    <NoWarn>$(NoWarn);CS1591;CA1819</NoWarn>


CA1819 indicates that public/protected properties shouldn't return arrays. The reason they note is that if the property doesn't return a copy of the array then people can (and likely will) start using that property as an indexed property to start poking data into places they possibly shouldn't.

I'm not a big stickler for stuff like this, but I feel like there's a decent point here - where we may have public properties (like in IComponentRegistration) that were returning IEnumerable<T> and the switch to T[] enables people to do things they maybe shouldn't be doing. I've read too many Raymond Chen posts where people write programs using APIs that were never intended for a particular purpose, but now it's all locked in.

I'd recommend not ignoring this and, instead, being very thoughtful about switching away from an interface and using [SuppressMessage] to pinpoint the specific choices and document why that choice was made.

From a larger perspective, moving from IList<T> to List<T>, from IEnumerable<T> to T[]... that stuff may limit our ability to change underlying implementations. For example, if we need to store things in a SortedList<T> or start using Span<T> in certain places, switching from an interface to a concrete type makes that implementation detail a breaking change.

tillig · 2018-06-05T15:41:13Z

src/Autofac/Builder/RegistrationData.cs

@@ -147,17 +147,17 @@ public IComponentLifetime Lifetime
        /// <summary>


The switch from interface to concrete implementation here is one of the areas I'm a little concerned about. Exposing the set of handlers like this makes any underlying improvement a breaking change.

tillig · 2018-06-05T16:24:59Z

I just made a pass @alexmg and added comments. I think most of the changes here are great. I am slightly concerned about the exposure of concrete backing types in public APIs (a la CA1819) though I recognize there is performance hit when using interfaces over concrete types. I want perf, but I don't want to lose sight of the fact that the code in the public APIs is more an agreement between Autofac and the consumer and not 100% "just Autofac code." I'm guessing the .NET BCL could also add perf improvements by switching to concrete types in places instead of interfaces, but that's not an option because it's written as a public API, not as app code owned entirely by the app developer.

…U,V>.

…istry.

…trationInfo.

…ncyDetector.

…ricDecoratorRegistrationSource.

tillig · 2018-08-16T15:35:28Z

I took some time and brought in some, but not all, of the changes here. From a public interface perspective, I really am concerned that using arrays in public properties may open doors to people shooting themselves in the foot by trying to change values in the array. Like, "I know, let's swap the order of this and that because reasons!" It sounds silly, but we do see a lot of this sort of thing come through on StackOverflow and other venues.

I'll mark the things I brought in.

tillig · 2018-08-16T15:39:53Z

src/Autofac/Features/OpenGenerics/OpenGenericRegistrationSource.cs

@@ -60,7 +60,7 @@ public IEnumerable<IComponentRegistration> RegistrationsFor(Service service, Fun
            if (registrationAccessor == null) throw new ArgumentNullException(nameof(registrationAccessor));

            Type constructedImplementationType;
-            IEnumerable<Service> services;
+            Service[] services;


I brought all the changes from this file into the develop branch.

tillig · 2018-08-16T15:40:06Z

src/Autofac/Features/OpenGenerics/OpenGenericServiceBinder.cs

@@ -41,7 +41,7 @@ internal static class OpenGenericServiceBinder
            IEnumerable<Service> configuredOpenGenericServices,
            Type openGenericImplementationType,
            out Type constructedImplementationType,
-            out IEnumerable<Service> constructedServices)
+            out Service[] constructedServices)


I brought all the changes from this file into the develop branch.

tillig · 2019-11-20T19:27:19Z

I think there are still things we may be able to pull in here for the Autofac 5 release, but I'm not sure we can really pull it in as-is anymore. Given that, I'm going to close the PR and tag @autofac/autofac so we can refer to it later when doing additional refactoring.

Belorus added 4 commits May 10, 2018 18:26

Removed some LINQ

fca2b93

Replaced some Collection interfaces with specific type declarations up to 30% reduction of memory allocation traffic during registration

Oopsey :)

d9f50b7

Returned hotfix

bae74ba

Removed few simple resolve-time memory allocations

d4503ea

alexmg reviewed Jun 5, 2018

View reviewed changes

tillig reviewed Jun 5, 2018

View reviewed changes

tillig added a commit that referenced this pull request Aug 16, 2018

Memory optimization from #919: Autofac.Builder.RegistrationBuilder.

0a3a081

tillig added a commit that referenced this pull request Aug 16, 2018

Memory optimization from #919: Autofac.Builder.RegistrationBuilder<T,…

7641af7

…U,V>.

tillig added a commit that referenced this pull request Aug 16, 2018

Memory optimization from #919: Autofac.Core.Registration.ComponentReg…

1ad26e0

…istry.

tillig added a commit that referenced this pull request Aug 16, 2018

Memory optimization from #919: Autofac.Core.Registration.ServiceRegis…

06a0bbf

…trationInfo.

tillig added a commit that referenced this pull request Aug 16, 2018

Memory optimization from #919: Autofac.Core.Resolving.CircularDepende…

fc17758

…ncyDetector.

tillig added a commit that referenced this pull request Aug 16, 2018

Memory optimization from #919: Autofac.Features.OpenGenerics.OpenGene…

1738b76

…ricDecoratorRegistrationSource.

tillig reviewed Aug 16, 2018

View reviewed changes

tillig mentioned this pull request Jan 30, 2019

Fix lock contention #941 #906 #953

Closed

tillig closed this Nov 20, 2019

tillig mentioned this pull request Jun 26, 2020

Reflection Constructor Binding Refactor #1155

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory traffic optimization #919

Memory traffic optimization #919

Belorus commented May 30, 2018 •

edited

tillig commented May 30, 2018

alexmg commented Jun 5, 2018

alexmg commented Jun 5, 2018

Belorus commented Jun 5, 2018

alexmg commented Jun 5, 2018

alexmg left a comment

alexmg Jun 5, 2018

tillig Jun 5, 2018

alexmg Jun 5, 2018

tillig Jun 5, 2018

alexmg Jun 5, 2018

tillig Jun 5, 2018

tillig Jun 5, 2018

alexmg Jun 5, 2018

tillig Jun 5, 2018

tillig Aug 16, 2018

alexmg Jun 5, 2018

alexmg Jun 5, 2018

alexmg Jun 5, 2018

alexmg Jun 5, 2018

tillig Aug 16, 2018

alexmg Jun 5, 2018

tillig Aug 16, 2018

alexmg Jun 5, 2018

tillig Aug 16, 2018

alexmg commented Jun 5, 2018

tillig Jun 5, 2018

tillig Jun 5, 2018

tillig commented Jun 5, 2018

tillig commented Aug 16, 2018

tillig Aug 16, 2018

tillig Aug 16, 2018

tillig commented Nov 20, 2019

		@@ -147,17 +147,17 @@ public IComponentLifetime Lifetime
		/// <summary>

Memory traffic optimization #919

Memory traffic optimization #919

Conversation

Belorus commented May 30, 2018 • edited

tillig commented May 30, 2018

alexmg commented Jun 5, 2018

alexmg commented Jun 5, 2018

Belorus commented Jun 5, 2018

alexmg commented Jun 5, 2018

alexmg 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

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

alexmg commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tillig commented Jun 5, 2018

tillig commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tillig commented Nov 20, 2019

Belorus commented May 30, 2018 •

edited