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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a breaking change. We could add an overload to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like these are to prevent the need to call the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm trying to put this together with the change from Could we leave this alone (as |
||
|
||
/// <summary> | ||
/// Gets the explicitly bound properties. | ||
/// </summary> | ||
public IList<Parameter> ConfiguredProperties { get; } = new List<Parameter>(); | ||
public List<Parameter> ConfiguredProperties { get; } = new List<Parameter>(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
} | ||
|
||
|
@@ -149,7 +149,7 @@ public static class RegistrationBuilder | |
Guid id, | ||
RegistrationData data, | ||
IInstanceActivator activator, | ||
IEnumerable<Service> services) | ||
Service[] services) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Pretty low risk. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I brought all the changes from this file into the |
||
{ | ||
return CreateRegistration(id, data, activator, services, null); | ||
} | ||
|
@@ -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)); | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
} | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I brought all the changes from this file into the |
||
|
||
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> | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No problem here. I think the |
||
{ | ||
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> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
|
||
|
@@ -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() }, | ||
}; | ||
|
@@ -147,17 +147,17 @@ public IComponentLifetime Lifetime | |
/// <summary> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is where a second overload to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It appears to me that the |
||
{ | ||
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); | ||
|
@@ -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> | ||
|
@@ -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)); | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or If this is intended to be a more granular way to control what gets built into the container, I'd recommend also including There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
{ | ||
/// <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 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
|
There was a problem hiding this comment.
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 returningIEnumerable<T>
and the switch toT[]
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>
toList<T>
, fromIEnumerable<T>
toT[]
... that stuff may limit our ability to change underlying implementations. For example, if we need to store things in aSortedList<T>
or start usingSpan<T>
in certain places, switching from an interface to a concrete type makes that implementation detail a breaking change.