Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory traffic optimization #919

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Autofac/Autofac.csproj
Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<AssemblyName>Autofac</AssemblyName>
Expand Down
6 changes: 0 additions & 6 deletions src/Autofac/Builder/ContainerBuildOptions.cs
Expand Up @@ -39,12 +39,6 @@ public enum ContainerBuildOptions
/// </summary>
None = 0,

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, an overload would be better.


/// <summary>
/// Does not call <see cref="IStartable.Start"/> on components implementing
/// this interface (useful for module testing.)
Expand Down
14 changes: 14 additions & 0 deletions src/Autofac/Builder/IRegistrationBuilder.cs
Expand Up @@ -278,5 +278,19 @@ public interface IRegistrationBuilder<out TLimit, out TActivatorData, out TRegis
/// </param>
/// <returns>A registration builder allowing further configuration of the component.</returns>
IRegistrationBuilder<TLimit, TActivatorData, TRegistrationStyle> WithMetadata<TMetadata>(Action<MetadataConfiguration<TMetadata>> configurationAction);

/// <summary>
/// Configure the services that the component will provide.
/// </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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// <summary>
/// Configure the services that the component will provide.
/// </summary>
/// <param name="service">Services to expose.</param>
/// <returns>A registration builder allowing further configuration of the component.</returns>
IRegistrationBuilder<TLimit, TActivatorData, TRegistrationStyle> As(Service service);
}
}
4 changes: 2 additions & 2 deletions src/Autofac/Builder/ReflectionActivatorData.cs
Expand Up @@ -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>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// <summary>
/// Gets the explicitly bound properties.
/// </summary>
public IList<Parameter> ConfiguredProperties { get; } = new List<Parameter>();
public List<Parameter> ConfiguredProperties { get; } = new List<Parameter>();
}
}
16 changes: 10 additions & 6 deletions src/Autofac/Builder/RegistrationBuilder.cs
Expand Up @@ -133,7 +133,7 @@ public static class RegistrationBuilder
builder.RegistrationStyle.Id,
builder.RegistrationData,
builder.ActivatorData.Activator,
builder.RegistrationData.Services,
builder.RegistrationData.Services.ToArray(),
builder.RegistrationStyle.Target);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Pretty low risk.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
return CreateRegistration(id, data, activator, services, null);
}
Expand All @@ -170,7 +170,7 @@ public static class RegistrationBuilder
Guid id,
RegistrationData data,
IInstanceActivator activator,
IEnumerable<Service> services,
Service[] services,
IComponentRegistration target)
{
if (activator == null) throw new ArgumentNullException(nameof(activator));
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good.

{
if (!ts.ServiceType.GetTypeInfo().IsAssignableFrom(limitType.GetTypeInfo()))
var asServiceWithType = ts as IServiceWithType;
if (asServiceWithType != null)
{
throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, RegistrationBuilderResources.ComponentDoesNotSupportService, limitType, ts));
if (!asServiceWithType.ServiceType.GetTypeInfo().IsAssignableFrom(limitType.GetTypeInfo()))
{
throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, RegistrationBuilderResources.ComponentDoesNotSupportService, limitType, ts));
}
}
}
}
Expand Down
Expand Up @@ -241,11 +241,27 @@ public RegistrationBuilder(Service defaultService, TActivatorData activatorData,
/// <returns>A registration builder allowing further configuration of the component.</returns>
public IRegistrationBuilder<TLimit, TActivatorData, TRegistrationStyle> As(params Type[] services)
{
return As(services.Select(t =>
t.FullName != null
? new TypedService(t)
: new TypedService(t.GetGenericTypeDefinition()))
.Cast<Service>().ToArray());
Service[] argArray = new Service[services.Length];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


for (int i = 0; i < services.Length; i++)
{
if (services[i].FullName != null)
argArray[i] = new TypedService(services[i]);
else
argArray[i] = new TypedService(services[i].GetGenericTypeDefinition());
}

return As(argArray);
}

/// <summary>
/// Configure the services that the component will provide.
/// </summary>
/// <param name="service">Service types to expose.</param>
/// <returns>A registration builder allowing further configuration of the component.</returns>
public IRegistrationBuilder<TLimit, TActivatorData, TRegistrationStyle> As(Type service)
{
return As(new TypedService(service));
}

/// <summary>
Expand All @@ -262,6 +278,20 @@ public RegistrationBuilder(Service defaultService, TActivatorData activatorData,
return this;
}

/// <summary>
/// Configure the services that the component will provide.
/// </summary>
/// <param name="service">Services to expose.</param>
/// <returns>A registration builder allowing further configuration of the component.</returns>
public IRegistrationBuilder<TLimit, TActivatorData, TRegistrationStyle> As(Service service)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem here. I think the AddService method has only been used by internal registration sources but has always been part of the public API.

{
if (service == null) throw new ArgumentNullException(nameof(service));

RegistrationData.AddService(service);

return this;
}

/// <summary>
/// Provide a textual name that can be used to retrieve the component.
/// </summary>
Expand Down
10 changes: 5 additions & 5 deletions src/Autofac/Builder/RegistrationData.cs
Expand Up @@ -41,7 +41,7 @@ public class RegistrationData
private bool _defaultServiceOverridden;
private Service _defaultService;

private readonly ICollection<Service> _services = new HashSet<Service>();
private readonly HashSet<Service> _services = new HashSet<Service>();

private IComponentLifetime _lifetime = new CurrentScopeLifetime();

Expand All @@ -56,7 +56,7 @@ public RegistrationData(Service defaultService)

_defaultService = defaultService;

Metadata = new Dictionary<string, object>
Metadata = new Dictionary<string, object>(1)
{
{ MetadataKeys.RegistrationOrderMetadataKey, SequenceGenerator.GetNextUniqueSequence() },
};
Expand Down Expand Up @@ -147,17 +147,17 @@ public IComponentLifetime Lifetime
/// <summary>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// Gets the handlers for the Preparing event.
/// </summary>
public ICollection<EventHandler<PreparingEventArgs>> PreparingHandlers { get; } = new List<EventHandler<PreparingEventArgs>>();
public List<EventHandler<PreparingEventArgs>> PreparingHandlers { get; } = new List<EventHandler<PreparingEventArgs>>();

/// <summary>
/// Gets the handlers for the Activating event.
/// </summary>
public ICollection<EventHandler<ActivatingEventArgs<object>>> ActivatingHandlers { get; } = new List<EventHandler<ActivatingEventArgs<object>>>();
public List<EventHandler<ActivatingEventArgs<object>>> ActivatingHandlers { get; } = new List<EventHandler<ActivatingEventArgs<object>>>();

/// <summary>
/// Gets the handlers for the Activated event.
/// </summary>
public ICollection<EventHandler<ActivatedEventArgs<object>>> ActivatedHandlers { get; } = new List<EventHandler<ActivatedEventArgs<object>>>();
public List<EventHandler<ActivatedEventArgs<object>>> ActivatedHandlers { get; } = new List<EventHandler<ActivatedEventArgs<object>>>();

/// <summary>
/// Copies the contents of another RegistrationData object into this one.
Expand Down
2 changes: 1 addition & 1 deletion src/Autofac/Builder/SingleRegistrationStyle.cs
Expand Up @@ -42,7 +42,7 @@ public class SingleRegistrationStyle
/// <summary>
/// Gets the handlers to notify of the component registration event.
/// </summary>
public ICollection<EventHandler<ComponentRegisteredEventArgs>> RegisteredHandlers { get; } = new List<EventHandler<ComponentRegisteredEventArgs>>();
public List<EventHandler<ComponentRegisteredEventArgs>> RegisteredHandlers { get; } = new List<EventHandler<ComponentRegisteredEventArgs>>();

/// <summary>
/// Gets or sets a value indicating whether default registrations should be preserved.
Expand Down
94 changes: 78 additions & 16 deletions src/Autofac/ContainerBuilder.cs
Expand Up @@ -131,6 +131,7 @@ public ContainerBuilder RegisterBuildCallback(Action<IContainer> buildCallback)
/// Create a new container with the component registrations that have been made.
/// </summary>
/// <param name="options">Options that influence the way the container is initialised.</param>
/// <param name="flags">Defines which default services should be enabled.</param>
/// <remarks>
/// Build can only be called once per <see cref="ContainerBuilder"/>
/// - this prevents ownership issues for provided instances.
Expand All @@ -139,11 +140,11 @@ public ContainerBuilder RegisterBuildCallback(Action<IContainer> buildCallback)
/// first create the container, then call Update() on the builder.
/// </remarks>
/// <returns>A new container with the configured component registrations.</returns>
public IContainer Build(ContainerBuildOptions options = ContainerBuildOptions.None)
public IContainer Build(ContainerBuildOptions options = ContainerBuildOptions.None, DefaultServiceFlags flags = DefaultServiceFlags.All)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where a second overload to Build might prevent breaking changes by mapping ExcludeDefaultModules to DefaultServiceFlags.None.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears to me that the DefaultServiceFlags (or FeatureToggles or ContainerFeatures or whatever) is an attempt at a more granular way to include or exclude default features... in which case, yeah, an overload where the current ContainerBuildOptions maps to some set of features would be better.

{
var result = new Container(Properties);
result.ComponentRegistry.Properties[MetadataKeys.ContainerBuildOptions] = options;
Build(result.ComponentRegistry, (options & ContainerBuildOptions.ExcludeDefaultModules) != ContainerBuildOptions.None);
Build(result.ComponentRegistry, flags);

if ((options & ContainerBuildOptions.IgnoreStartableComponents) == ContainerBuildOptions.None)
StartableManager.StartStartableComponents(result);
Expand Down Expand Up @@ -206,7 +207,7 @@ public void Update(IContainer container, ContainerBuildOptions options)
[Obsolete("Containers should generally be considered immutable. Register all of your dependencies before building/resolving. If you need to change the contents of a container, you technically should rebuild the container. This method may be removed in a future major release.")]
public void Update(IComponentRegistry componentRegistry)
{
this.UpdateRegistry(componentRegistry);
UpdateRegistry(componentRegistry);
}

/// <summary>
Expand All @@ -222,10 +223,10 @@ public void Update(IComponentRegistry componentRegistry)
internal void UpdateRegistry(IComponentRegistry componentRegistry)
{
if (componentRegistry == null) throw new ArgumentNullException(nameof(componentRegistry));
Build(componentRegistry, true);
Build(componentRegistry, DefaultServiceFlags.None);
}

private void Build(IComponentRegistry componentRegistry, bool excludeDefaultModules)
private void Build(IComponentRegistry componentRegistry, DefaultServiceFlags flags = DefaultServiceFlags.None)
{
if (componentRegistry == null) throw new ArgumentNullException(nameof(componentRegistry));

Expand All @@ -234,28 +235,89 @@ private void Build(IComponentRegistry componentRegistry, bool excludeDefaultModu

_wasBuilt = true;

if (!excludeDefaultModules)
RegisterDefaultAdapters(componentRegistry);
RegisterDefaultAdapters(componentRegistry, flags);

foreach (var callback in _configurationCallbacks)
callback.Callback(componentRegistry);
}

private void RegisterDefaultAdapters(IComponentRegistry componentRegistry)
private void RegisterDefaultAdapters(IComponentRegistry componentRegistry, DefaultServiceFlags flags)
{
this.RegisterGeneric(typeof(KeyedServiceIndex<,>)).As(typeof(IIndex<,>)).InstancePerLifetimeScope();
componentRegistry.AddRegistrationSource(new CollectionRegistrationSource());
componentRegistry.AddRegistrationSource(new OwnedInstanceRegistrationSource());
componentRegistry.AddRegistrationSource(new MetaRegistrationSource());
componentRegistry.AddRegistrationSource(new LazyRegistrationSource());
componentRegistry.AddRegistrationSource(new LazyWithMetadataRegistrationSource());
componentRegistry.AddRegistrationSource(new StronglyTypedMetaRegistrationSource());
componentRegistry.AddRegistrationSource(new GeneratedFactoryRegistrationSource());
if ((flags & DefaultServiceFlags.Index) == DefaultServiceFlags.Index)
this.RegisterGeneric(typeof(KeyedServiceIndex<,>)).As(new[] { typeof(IIndex<,>) }).InstancePerLifetimeScope();
if ((flags & DefaultServiceFlags.Collections) == DefaultServiceFlags.Collections)
componentRegistry.AddRegistrationSource(new CollectionRegistrationSource());
if ((flags & DefaultServiceFlags.Owned) == DefaultServiceFlags.Owned)
componentRegistry.AddRegistrationSource(new OwnedInstanceRegistrationSource());
if ((flags & DefaultServiceFlags.Meta) == DefaultServiceFlags.Meta)
componentRegistry.AddRegistrationSource(new MetaRegistrationSource());
if ((flags & DefaultServiceFlags.Lazy) == DefaultServiceFlags.Lazy)
componentRegistry.AddRegistrationSource(new LazyRegistrationSource());
if ((flags & DefaultServiceFlags.LazyWithMeta) == DefaultServiceFlags.LazyWithMeta)
componentRegistry.AddRegistrationSource(new LazyWithMetadataRegistrationSource());
if ((flags & DefaultServiceFlags.StronglyTypedMeta) == DefaultServiceFlags.StronglyTypedMeta)
componentRegistry.AddRegistrationSource(new StronglyTypedMetaRegistrationSource());
if ((flags & DefaultServiceFlags.GeneratedFactory) == DefaultServiceFlags.GeneratedFactory)
componentRegistry.AddRegistrationSource(new GeneratedFactoryRegistrationSource());
}

private List<Action<IContainer>> GetBuildCallbacks()
{
return (List<Action<IContainer>>)Properties[BuildCallbackPropertyKey];
}
}

[Flags]
public enum DefaultServiceFlags
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem with the concept but could do with a better name. Perhaps something like FeatureToggles.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or ContainerFeatures.

If this is intended to be a more granular way to control what gets built into the container, I'd recommend also including StartableComponents as a flag so it can do everything ContainerBuildOptions can do and more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should also be in its own file, not attached to the bottom of ContainerBuilder.cs.

{
/// <summary>
/// None
/// </summary>
None = 0,

/// <summary>
/// Index
/// </summary>
Index = 1,

/// <summary>
/// Collection
/// </summary>
Collections = 2,

/// <summary>
/// Owned
/// </summary>
Owned = 4,

/// <summary>
/// Meta
/// </summary>
Meta = 8,

/// <summary>
/// Lazy
/// </summary>
Lazy = 16,

/// <summary>
/// LazyWithMeta
/// </summary>
LazyWithMeta = 32,

/// <summary>
/// StronglyTypedMeta
/// </summary>
StronglyTypedMeta = 64,

/// <summary>
/// GeneratedFactory
/// </summary>
GeneratedFactory = 128,

/// <summary>
/// All
/// </summary>
All = Index | Collections | Owned | Meta | Lazy | LazyWithMeta | StronglyTypedMeta | GeneratedFactory
}
}
Expand Up @@ -66,7 +66,7 @@ public class ConstructorParameterBinding
/// <param name="context">Context in which to construct instance.</param>
public ConstructorParameterBinding(
ConstructorInfo ci,
IEnumerable<Parameter> availableParameters,
Parameter[] availableParameters,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A breaking change but I don't see this class being called directly so probably fine.

IComponentContext context)
{
if (ci == null) throw new ArgumentNullException(nameof(ci));
Expand Down