This project is read-only.

Ioc library Experiment

Jan 15, 2014 at 12:06 AM
Hello Steven

I like your SimpleInjector library. It's simple and has many usefull features. I've created a ioc library: https://github.com/geovaly/Ioc because I was seeking some flexibile ways to create custom lifetimes. It's more like an experiment. Can you please take a look and tell me your impression. I was wondering if we can merge that library with SimpleInjector. :)

Valy
Jan 15, 2014 at 11:53 AM
Hi Valy,

I looked at your Github repo, but I'm not sure what limitations of Simple Injector you came across. What is the problem with creating custom lifestyles you are trying to solve?
Jan 15, 2014 at 11:11 PM
What I don't like:
  • Performance for lifetime scoping: to resolve a servive in a lifetime scope you need to get the current scope from a ThreadLocal field and then you look up in a dictionary to get the instance. This is also true when injecting the arguments of the constructor. So if you have a service that depends on 10 services from the lifetime scope you will have 10 hits on ThreadLocal and 10 dictionary lookups. The trick that I use to avoid ThreadLocal is to pass the lifetime scope as input to instance creators like this:
 delegate TService InstanceCreator<out TService>(ILifetimeScope lifetimeScope) where TService : class;
To avoid accessing a dictionary on each resolve is to have fields/properties on the lifetime scope class. To get a service from the filetime scope you will need to access the specific field of the lifetime.
  • The codebase of the Integration.Wcf, Extensions.LifetimeScoping, Integration.Web. Because you get the current lifetime scope from the ThreadLocal you need to define in all projects the lifetime scope and the dictionary. (In Integration.Web you use the the dictionary from HttpContext). What if in the future you will need to integrate with another technology. You have to write another one similar. I thing is more simple and flexible to have one lifetime scope defined in one place and use it with what techonology you want. This means to pass the lifetime scope as input.
  • What if you want 2 lifetime scopes with different services? Or one lifetime scope to be included in another? Or the user to have more control when the service is requested from the lifetime scope? I know that you can simulate this but I think it's best to be explicit.
The downside on avoiding the dictionary lookup is that you need to define your service registrations as fields not as methods on the container.
Jan 15, 2014 at 11:15 PM
Edited Jan 15, 2014 at 11:21 PM
So what I suggest is
  1. pass the lifetime scope as input to instance creators. (avoiding thread local access)
  2. register services as fields to the lifetime scope class. (avoiding dictionary lookup)
  3. maintain a hierarchy between different lifetime scopes.
Jan 16, 2014 at 11:21 AM
Edited Jan 16, 2014 at 6:11 PM
I agree that the performance of scoped lifestyles is far from optimal. This holds for all currently available scoped lifestyles (LifetimeScopeLifestyle, WebRequestLifestyle, WcfOperationLifestyle), and future scoped lifestyles (such as Execution Context and Web API).

UPDATE: The words 'scoped' were missing in the first paragraph.

The overall similarity between all those lifestyles however is that there is at most one instance per object graph. So instead of implementing some sort of ILifetimeScope interface and passing it along the delegates down the object graph, I think it's better to do the optimizations by altering the built Expression tree just before it is compiled down to IL. This might result in even better performance and prevents any breaking changes.

The trick will probably be to flag a Lifestyle as IPerObjectGraph or have an PerObjectGraphLifestyle base class of some sort to notify the framework that the object graph can be optimized.
Jan 16, 2014 at 11:24 AM
Edited Jan 16, 2014 at 11:25 AM
This discussion has been copied to a work item, but let's continue the discussion here.
Jan 16, 2014 at 4:23 PM
Edited Jan 16, 2014 at 4:37 PM
I think you mean to have a variable in expression tree at the begining of the block that will be equal with the one from ThreadLocal? So you have only one access with ThreadLocal. This because you don't want breaking changes because you resolve every service type from the container. In autofac resolving lifetime scope services is done from the instance of the lifetime scope :
using (var lifetimeScope = container.BeginLifetimeScope())
    {
        var r = lifetimeScope.Resolve<IMyResource>();
         ...
    }
In this case it easy to pass the instance of the lifetime scope as input :)
Jan 16, 2014 at 4:31 PM
The input of the lifetime scope does not necessarily need to be a interface. You can compile delegate as
delegate TService InstanceCreator<in TLifetimeScope, out TService>(TLifetimeScope lifetimeScope) where TService : class;
You can compily only delegates that resolve services from the lifetime scope. The transient and singletons don't need to have that input.
Jan 16, 2014 at 4:32 PM
And for having fields in the lifetime scope class for each service that you want to register. Why you don't like it?
Jan 16, 2014 at 6:35 PM
You are right about the design of Autofac; you resolve from the scope, instead of from the container. Such design has a few very clear advantages, such as the possibility to move this scope from thread to thread, while with Simple Injector the LifetimeScope is fixed to a single thread. I must admit that think Autofac's design is very good for this particular issue, but I explicitly chose a different design with Simple Injector. Having to pass on an hang on to a lifetimeScope instance does increase the complexity and makes it much easier to go wrong. The author of Autofac has a few very long blog posts to explain this behavior and how users should handle this. I've read these articles multiple times but decided that Autofac's design didn't meet Simple Injector's philosophy. Being able to call the container directly is much easier to grasp and much harder to do wrong. Changing this design would of course be a breaking change.

I think that using expression variables is the way to go. I just prototyped this and it works pretty nicely. This is how such an expression tree would look like:
Func<HomeController> factory = () =>
{
    IRepository value1 = lifetimeScopeRegistration.GetInstance();
    return new HomeController(
        value1,
        new SomeQueryHandler(value1),
        new SomeCommandHandler(value1));
};
Internally this is built using the System.Linq.Expressions API using a BlockExpression.

Thanks for bringing this up.
Jan 16, 2014 at 9:01 PM
Edited Jan 16, 2014 at 9:04 PM
LifetimeScopeRegistration.GetInstance() will hit ThreadLocal? If yes then if you have many services you will hit ThreadLocal for each service.
You can do like this:
Func<HomeController> factory = () =>
{
    var lifetimeScope = LifetimeScopeManager.GetCurrentLifetimeScope(); // hit ThreadLocal
    IRepository value1 = lifetimeScope.GetInstance<IRepository>();  //From Dictionary
    IService value2 = lifetimeScope.GetInstance<IService>(); //From Dictionary
    return new HomeController(
        value1,
        new SomeQueryHandler(value1),
        new SomeCommandHandler(value2));
};

Jan 16, 2014 at 9:21 PM
Edited Jan 16, 2014 at 9:44 PM
My opinion is that it's more explicit to pass the lifetime scope instance. If it's depending on that instance why not make it explicit for the user. What if I want to have 2 lifetime scopes opened on the same time? For example in multi-tenant applications I will want a single instance for the container but for each tenant a lifetime scope that will simulate singletons for each tenant. And what do you do for lifetimes hierarchies: http://stackoverflow.com/questions/16796407/need-guidance-on-autofac-custom-lifetimescopes-vs-multi-tenancy

If you go this path you can make a interface like this:
public interface IInstanceProvider
{
     TService GetInstance<TService>() where TService : class;
}
Then implement it by container and lifetime scope. And you can make a CommandBus this way:
    public class CommandBus : ICommandBus
    {
        private readonly IInstanceProvider _instanceProvider;

        public CommandBus(IInstanceProvider instanceProvider)
        {
            _instanceProvider = instanceProvider;
        }

        private ICommandHandler<T> GetHandler<T>() where T : ICommand
        {
            return _instanceProvider.GetInstance<ICommandHandler<T>>();

        }
        public void SubmitCommand<TCommand>(TCommand command) where TCommand : ICommand
        {
            GetHandler<TCommand>().Handle(command);
        }
    }
When you will resolve the CommandBus it will be inject with the current lifetime scope. Ok it's my opinion. Don't get me wrong. Maybe you can implement both variants.
Jan 16, 2014 at 9:25 PM
Edited Jan 16, 2014 at 9:36 PM
Another trick that I use in my library is SingletonContainer. Please take a look. It's a static class that wraps the container and use a cache that will avoid lookup in dictionary when getting an instance. In normal case a user will have one conainter per application. It only work when you get instance by the generic method. You get my point. ;)
Jan 16, 2014 at 10:11 PM
Loading the current lifetime scope once per object graph would indeed be a much greater optimization, especially since a single object graph will often be split up in multiple smaller delegates, since most lifestyles (about everything except transient and singleton) trigger this. Such optimization however is not something that is very easy to do, because of two reasons: 1. The core library knows nothing about LifetimeScope; the LifetimeScope is in a different dll. 2. There are more (scoped) lifestyles that need this optimization (currently three, and in the future even more); the optimization can therefore not be made for the lifetimeScope solely.

So in order to pull such optimization of, that would mean that each lifestyle must be able to hook into the container with a callback that allows this optimization, or there must be some generic method on the lifestyle class that describes how the cache should be loaded. An interesting idea. I have to think about this.

> If it's depending on that instance why not make it explicit for the user.

That would be a breaking change and a rather large one, since the whole idea behind Simple Injector is that users can register Func<T> delegates as factories. Inside those delegates users are allowed call container.GetInstance. When users need to start calling scope.GetInstance, this also means that the delegates need to be Func<IScopedContainer, T> or something similar. And although this is a design that makes sense for Autofac, it doesn't for Simple Injector. It follows a different philosophy.

> or example in multi-tenant applications I will want a single instance for the container but for each tenant a lifetime scope that will simulate singletons for each tenant.

In that case it would be very rare to handle multiple tenants in the same request, so the solution would probably be to have one container per tenant and select the right container on that request. If you do this with a single container, don't use the lifetime scope as boundary for the tenant. This works for Autofac, since the lifetime scope in Autofac can be moved from thread to thread and is thread-safe. In Simple Injector, lifetime scope is not thread-safe and can only be used from one thread. The Simple Injector lifetime scope has little similarity with Autofac's lifetime scope.

> Ok it's my opinion. Don't get me wrong.

Your ideas are very reasonable, and it is clear you are a very experienced user of DI containers and you are probably very familiar with Autofac. The designer of Autofac, Nicholas Blumhardt, is a very smart guy, and I have lots of respect for him. For Simple Injector however, I deliberately chose a different design and that is something I still stand by (and simply can't change without losing my complete user base). And don't forget: what advantage would Simple Injector have over Autofac if it followed the exact design? There would be none.

> Another trick that I use in my library is SingletonContainer.

That SingletonContainer is a Service Locator. Both Ninject and StructureMap contain such facade and I deliberately left this out of Simple Injector because: 1. It will almost always be abused, application code should not call the container, and 2. It's just too easy to write this as application developer if you really want.
Jan 16, 2014 at 11:48 PM
Edited Jan 17, 2014 at 4:35 AM
That SingletonContainer is a Service Locator.
To be a Service Locator it depends on how it's use it. The Container also can be a Service Locator if it's not used from the Composition Route. http://blog.ploeh.dk/2011/08/25/ServiceLocatorrolesvs.mechanics/ . The CommandBus in the above example ( this class will be part of the composition Route, it's an infrastructure class, so will not count as a service locator) will be very happy to not hit the dictionary when he handle a command :)

Regarding that the core library knows nothing about LifetimeScope. You need to hookup the begining of the block expression when compiling a delegate with CompilationHelpers. You can have a virtual method in the LifeStyle class and override it when you need it like this:
public abstract class Lifestyle
{
   public virtual IEnumerable<Expression> BuildInitBlockExpressions(..?..){ return Enumerable.Empty<Expression>(); }
}
Next you need to modify CompilationHelpers to build a delegate that use BuildInitBlockExpressions from the Lifestyles that the current service depends on.
I think is better to make CompilationHelpers a class of it's on. You can use strategy pattern (IExpressionCompilationStrategy).

This you can make with your current design. What I don't like is to have a scoped lifestyle class ( and manager class, scope class, registration class ) for each technology (WCF,WEB, Custom). You have code duplication. You need to get the abstraction. The scoped lifestyle is the same. The scoped lifestyle must have a start and an end. This will not change. What changes is when will start and when will end. The when is the techonology that need to be separated. So what I will want to do is to find the abstraction and include it in the core library.

The Simple Injector is great. But "The day you think there is no improvements to be made is a sad one for any player." I will change "player" with "developer" :)
Thanks for your feedback Steven. What do you think?
Jan 17, 2014 at 12:23 AM
Edited Jan 17, 2014 at 12:30 AM
the whole idea behind Simple Injector is that users can register Func<T> delegates as factories. ...
If you register services as fields in lifetime scope class you can use Func<T> delegates and not Func<IScopedContainer, T>. In my library I have a LazyCustomRegister class witch takes a Func<T> in constructor. I know... this will be a more greater breaking change. ... I think I am in love with those register fields :)))
Jan 17, 2014 at 12:40 AM
Edited Jan 17, 2014 at 3:00 AM
Another think I don't like is that registrations have to depend on container class. If you refractor the CompilationHelpers as a strategy class, you can inject it in the registration class. If you want to get to the options of the container this means that you can have other strategies that can be injected.

Also CompilationHelpers don't need to depend on Container. You pass the container only to get the ModuleBuilder. The law of demeter is violated. After refactoring the CompilationHelpers, you can inject the ModuleBuilder in the constructor from the container.


Container is the public API. Is the main coordinator of your library. It must know about all classes. And other classes don't need to know about it. it will make maintenance easier.
Jan 17, 2014 at 10:18 AM
> To be a Service Locator it depends on how it's use it.

That is of course correct. It's not a service locator when you use it inside the Composition Root. But you don't need it inside the Composition Root as well, since it is very easy to pass on the container itself onto a class (your CommandBus for instance). As a matter of fact, I have never yet seen seen a problem that that could only be solved by using such static class.

> What I don't like is to have a scoped lifestyle class ... for each technology

That's a very valid point. An ScopedLifestyle base class has been added because of this and in the previous release more shared behavior was moved into this ScopedLifestyle class. That's clearly a smell. This is probably even something we can change without breaking peoples code.

> The day you think there is no improvements to be made is a sad one for any player.

That's absolutely true. But please don't forget that adding features is not always an improvement. Fixing design problems however is an improvement, but on the other hand introducing breaking changes again isn't. So that's something that has to be balanced constantly. We're still working hard on Simple Injector and your feedback is very much appreciated.

> this will be a more greater breaking change. ... I think I am in love with those register fields :)))

I noticed :-), but yes, that will be a breaking change that is too big and risky for me.

> Another think I don't like is that registrations have to depend on container class

The Registration base class is the class that builds the transient Expression tree for a particular registration (derived types add the lifestyle). The Registration base class itself queries the Container a lot for certain policies (constructor resolution, property selection, initializers, etc). Although building the expression could be moved out of the Registration, that is its job. Moving this behavior out of the Registration into a strategy would be useful when this strategy needs to swap. But this is something that hasn't been needed yet, and I like to delay these designs until I got enough information. And even if we move this behavior out of the Registration, the Registration base class, the Registration.Container property is protected, which means that its part of the public API. Removing it would again be a breaking change (probably not a big one though).

> The law of demeter is violated. Container is the public API. Is the main coordinator of your library. It must know about all classes. And other classes don't need to know about it.

That's a good point. It has been a long time since I've had such a thorough code review on the Simple Injector code base.
Jan 17, 2014 at 11:18 AM
Edited Jan 17, 2014 at 12:03 PM
About your proposed optimization improvement, you posted the following idea:
Func<HomeController> factory = () =>
{
    var lifetimeScope = LifetimeScopeManager.GetCurrentLifetimeScope(); // hit ThreadLocal
    IRepository value1 = lifetimeScope.GetInstance<IRepository>();  //From Dictionary
    IService value2 = lifetimeScope.GetInstance<IService>(); //From Dictionary
    return new HomeController(
        value1,
        new SomeQueryHandler(value1),
        new SomeCommandHandler(value2));
};
Let's improve this a bit. The problem with this code is that all instances are fetched directly on the top of the method. In most cases this is not a problem, since placing the lifetimeScope.GetInstance calls inside the constructor parenthesis of the HomeController would have the exact same effect (as long as the order in which the values are created is maintained. Since users however are allowed to transform expression trees, they are allowed to delay or skip creating instances based on some condition, for instance by injecting an Expression.Condition into the tree. There's actually a code sample for decorators that does this (look here). The RegisterRuntimeDecorator gives the user the opportunity to register a decorator that gets applied conditionally, based on a predicate that is evaluated every time the object graph is constructed (where the built-in RegisterDecorator method of Simple Injector only evaluates the predicate once - for performance of course).

Although the RegisterRuntimeDecorator extension method would keep working with the above code snippet, it will cause two branches to be built. One containing the decorator and one without. This again can hinder performance just as much as the optimization we're doing, but more importantly, users shouldn't be allowed to expect that that the optimizations Simple Injector does, does not change the meaning of their intent, which is: to execute one of the condition's branches based on a predicate instead of always executing both branches but using just one.

Besides this code example, even Simple Injector's HybridLifestyle make use of Expression.Condition and this allows switching between two lifestyles based on a predicate. Preloading both object graphs here would almost always lead to problems, since thise method is often used to fall back to a different lifestyle. For instance, use the lifetime scope if the web request is unavailable. If both graphs are preloaded, resolving the instance would certainly fail in case the web request is unavailable.

We can fix this by changing the code to the following:
Func<HomeController> factory = () =>
{
    var lifetimeScope = LifetimeScopeManager.GetCurrentLifetimeScope(); // hit ThreadLocal
    var value1 = new Lazy<IRepository>(() => lifetimeScope.GetInstance<IRepository>(), isThreadSafe: false);  //From Dictionary
    var value2 = new Lazy<IServicer>(() => lifetimeScope.GetInstance<IService>(), isThreadSafe: false); //From Dictionary
    return new HomeController(
        value1.Value,
        new SomeQueryHandler(value1.Value),
        new SomeCommandHandler(value2.Value));
};
By using the System.Lazy<T> class we can postpone the creation of those instances which has two advantages. First of all this solves the previously noted problem. And second, we don't have to worry about the order in which those lazys are created.

But this by itself raises other problems. For sake of performance, Simple Injector prevents the creation of reference types other than the objects that are actually used in the object graph. The example above creates 6 reference types that are created just for the construction of the graph, but are disposed later on. Not only are two Lazy<T> instances created, but two new delegate's are constructed as well, and since those delegates use a closure (the lifetimeScope variable comes from the outside), two extra class instances are created (in case C# would compile this for us). This forces extra pressure on the garbage collector and should be prevented at all costs. Besides, the use of these closures would probably be rather painful to implement using Expression trees.

So the use of Lazy<T> is out of the picture. There's probably not much choice but to implement a Lazy-like structure; let's call it struct ScopedLazy<T>:
Func<HomeController> factory = () =>
{
    var scope1 = LifetimeScopeManager.GetCurrentLifetimeScope(); // hit ThreadLocal
    var value1 = new ScopedLazy<IRepository>(scope1, scope => registration1.GetInstance(scope));
    var value2 = new ScopedLazy<IService>(scope1, scope => registration2.GetInstance(scope));
    return new HomeController(
        value1.Value,
        new SomeQueryHandler(value1.Value),
        new SomeCommandHandler(value2.Value));
};
This effectively solves the previously stated problems, because:
  1. ScopedLazy<T> is a struct and will be pushed on the stack.
  2. The scope is passed in to the constructor of ScopedLazy<T> and passed on to the registered delegate. This means that there is no closure which means that no closure class instances need to be created.
  3. Since the used registration objects are constant, the passed in delegates can be compiled down in advance and simply passed on as pointers. This prevents any allocations for the delegates as well.
The only downside of this approach is that ScopedLazy<T> must be a public type in the SimpleInjector core library. When compiling delegates into anonymous assemblies (using ModuleBuilder), those types must (of course) be publicly accessible. So ScopedLazy<T> would become part of the public API of Simple Injector. That's certainly something I don't like, but it is something I can live with.
Jan 17, 2014 at 11:24 AM
Edited Jan 18, 2014 at 1:25 PM
But you don't need it inside the Composition Root as well, since it is very easy to pass on the container itself onto a class (your CommandBus for instance)
The CommandBus is part of the Composition Root. In this article http://blog.ploeh.dk/2011/09/19/MessageDispatchingwithoutServiceLocation/ Mark Seemann wrote that " I even made it a private nested class in my Composition Root to underscore the point."
As a matter of fact, I have never yet seen seen a problem that that could only be solved by using such static class.
The static class in my opinion is only a wrapper that has a cache for the container . It doesn't solve specific problems that container can't. You can choose to use it or not. CommandBus probabily if he need more speed when resolving command handlers then he can use that static class instead of the container.
It's just too easy to write this as application developer if you really want.
It's easy only if your container has a public method like: Func<T> GetInstanceCreator<T>() where T : class. In my library I made it internal because the static class is in the pachage.
Jan 18, 2014 at 1:21 PM
Edited Jan 18, 2014 at 1:39 PM
I did not see your last post until now :) The idea with ScopedLazy looks verry good. I like it. You want to get maximum performance you can get ;) Cool. I look forward to see your next library versions ;).

How do you think registration.GetInstance(scope) looks like? I think you can simulate the dictionary that LifetimeScope has with an array. Because we know all services that a lifetime scope has you can assign them an index from 0 to n. So it can look like this:
TService GetInstance(LifetimeScope scope)
{
    return scope[ this.index ] as TService;
}
A little fast then a dictionary and consume min memory.
Jan 18, 2014 at 5:15 PM
Edited Jan 19, 2014 at 7:31 PM
> You want to get maximum performance you can get

If we're going to optimize, we'd better optimize good :-)

_> How do you think registration.GetInstance(scope) looks like?_

My idea was to use registration.GetInstance(scope) for the fallback scenario where scope == null. In that case either 1. a transient instance should be thrown in case the container is verifying, or 2. an exception should be thrown. But I noticed that the code could be simplified by letting the ScopedLazy make the call internally. This prevents the GetInstance(scope) method from having to be public (since the method needs to be public when compiled in an anonymous assembly) and makes the expression tree easier to build (and minimizes the amount of code to compile). In my latest experiment I even moved the GetInstance(scope) method to the Scope class. As you see, I'm still heavily experimenting with this.

I'm also experimenting with cleaning up the scoped lifestyles as you suggested. This is actually quite tricky, since I want to do this in a way that existing user code does not break (and there are quite a few developers who wrote their own lifestyle).

Btw, I noticed another flaw in my previous code snippet. In the code snippet the given scope is eagerly loaded, but this is incorrect for the same reason prematurely loading scoped instances is a problem. The scope might be inside a Condition expression and never have to load, but we still have the performance hit of getting the scope from a ThreadLocal<T> or HttpContext.Current, etc.

So the Scope should be lazy-loaded as well:
Func<HomeController> factory = () =>
{
    var scope1 = new LazyScope((Func<Scope>)scopeFactory1);
    var value1 = new LazyScopedRegistration<IRepository>(scope1, reg1);
    var value2 = new LazyScopedRegistration<IService>(scope1, reg2);
    
    return new HomeController(
        value1.Instance, // hit ThreadLocal, Hit dictionary
        new SomeQueryHandler(value1.Instance),
        new SomeCommandHandler(value2.Instance)); // Hit dictionary
};
The code above wraps the scope factory in a LazyScope struct and the registrations in a LazyScopedRegistration struct. The call to GetInstance(scope) is moved inside the LazyScopedRegistration class.
Jan 19, 2014 at 6:56 PM
Edited Jan 19, 2014 at 6:59 PM
I was looking on RegisterRuntimeDecorator. I didn't investigate but if the service is in the scoped lifestyle, the decorator must be also in the same lifestyle, yes?

So in your example above... the value1.Instance must be wrapped only one... So the condition that check if decorator is applying must be executing only once. If not you will have 2 decorators that wraps a single scoped service.

I am right?
Jan 19, 2014 at 7:51 PM
It is very well possible, and common, to have a decorator with a shorter lifestyle as its decoratee. Imagine a command handler that is registered as Per Web Request and wrapped with a decorator that depends on the decoratee and a ILogger service (registered as Transient SqlLogger) and adds logging as cross-cutting concern. Since the lifestyle of a component should always be shorter or equal to that of all its dependencies, the decorator must have a lifestyle of Transientas well, and it should not be registered as Per Web Request.

In other words, if the decorated service is injected multiple times, this might mean that multiple decorators need to be created. If, on the other hand, the decorator itself is registered as scoped lifestyle, the decorated service will be compiled in a separate delegate and only the decoratee will be stored in the local variable. Any lifestyle other than transient and singleton force the generation of a delegate and hide the creation of that part of the object graph behind that delegate.

This inevitably means that the optimization we are currently talking about stops at such delegate. If an object graph contains two components that have a scoped lifestyle, the total object graph will be constructed using three delegates. If the root object depends on the first component and that first component depends on the second component, this means that -using the described optimization- the delegate for that first component again needs to load the scope again (thus hitting ThreadLocal again in case of the LifetimeScope).

Preventing this second request for the scope is probably possible, but let's see where the proposed optimization brings us first.
Jan 19, 2014 at 8:58 PM
Ok Steve... Let's see ;)
Jan 26, 2014 at 1:58 AM
I checked in changeset 106408 which improves the ScopedLifestyle API as you suggested and adds the optimization that we discussed. This was quite a lot of work, but I'm pretty pleased with the result and I managed to do this without introducing breaking changes.

Besides the ScopedLifestyle class there's now a Scope class that acts as cache and allows disposing instances when the scope ends. The Lifetime Scope lifestyle uses this Scope class, and although the Lifetime Scope is still addicted to a single thread, the Scope by itself is not. So it will be quite easy to create a custom scoped lifestyle that does allow an asynchronous programming model.

Thanks again for this discussion. If you got any other feedback or ideas, please let me know.
Jan 28, 2014 at 5:21 AM
Edited Jan 28, 2014 at 5:24 AM
Hi Steve,

I'm happy to hear this good news from you ;)

After a quick look, first think I saw is this comment :) :
//Only depend on this type when you want to be absolutely sure a future 
    /// version of the framework will break your code.
I think you didn't make them internal because the dynamic assembly needs access to them. You can define InternalsVisibleTo from 0 to your magic number 5:
[assembly: InternalsVisibleTo("SimpleInjector.Compiled_1")]
[assembly: InternalsVisibleTo("SimpleInjector.Compiled_2")]
...
and then exclude them from InternalUseFinderVisitor. Why do you have one dynamic assembly per container? It's more efficent than having only one ?

Secondly I saw the dictionary cachedInstances in the scope class. I have replace it with an array in my code and the tests pass. It's a little more efficient:
public class Scope : IDisposable
    {
        private static readonly object[] EmptyCachedInstances = new object[0];
        private object[] cachedInstances = EmptyCachedInstances;
      
         ... 
    }

 internal static TService GetInstance<TService, TImplementation>(
            ScopedRegistration<TService, TImplementation> registration, Scope scope)
            where TImplementation : class, TService
            where TService : class
      {
            ...

            if (scope.cachedInstances.Length < registration.Lifestyle.RegistrationsCount)
            {
                var newCachedInstances = new object[registration.Lifestyle.RegistrationsCount];
                Array.Copy(scope.cachedInstances, newCachedInstances, scope.cachedInstances.Length);
                scope.cachedInstances = newCachedInstances;
            }

            object instance = scope.cachedInstances[registration.Id];

            if (instance == null)
            {
                instance = registration.InstanceCreator();
                scope.cachedInstances[registration.Id] = instance;
                ...
            }
    }

 internal sealed class ScopedRegistration<TService, TImplementation> : Registration
        where TImplementation : class, TService
        where TService : class
{
       
        internal int Id
        {
            get;
            private set;
        }

        internal ScopedRegistration(int id, ...)
        {
           Id = id;
        }
}

 public abstract class ScopedLifestyle : Lifestyle
 {    
        private int _nextRegistrationId;

        internal int RegistrationsCount
        {
            get { return _nextRegistrationId; }
        }

         protected override Registration CreateRegistrationCore<TService>(... )
        {
            return new ScopedRegistration<TService, TService>(_nextRegistrationId++, ...);
        }
}
What do you think?

PS. I betrayed my registered fields on the lifetime class :)) Now I am trying to build something like AutoFac:
 var lifetimeScope = container.BeginLifetimeScope(tag: "session", state: new Session(...) )
{
     var instance = lifetimeScope.Resolve<ICommand>();
}
Similar to having a class for a lifetime: we have the state of the class and the type ( tag ).

And I want to register like this:
container.Register<Session>( lifetime => (Session) lifetime.State , Lifestyle.SingletonPerLifetimeScope ) );

container.Register<IFoo, FooImpl1>( Lifestyle.Singleton );
container.Register<IGoo ,GooImpl1>( Lifestyle.Transient );

container.Register<IFoo, FooImpl2>( Lifestyle.SingletonPerLifetimeScope );
container.Register<IGoo, GooImpl2>( Lifestyle.TransientPerLifetimeScope );

container.Register<IFoo, FooImpl3>(Lifestyle.TransientPerMatchedLifetimeScope("tag1","tag2"));
container.Register<IGoo, FooImpl3>(Lifestyle.SingletonPerMatchedLifetimeScope("tag1","tag2"));
So the implementation will be choosen from the current lifetime.
Feb 1, 2014 at 4:34 PM
Edited Feb 1, 2014 at 4:35 PM
UnregisteredTypeEventArgsCallHelper.GetInstance -> type is not public.

>> You can define InternalsVisibleTo from 0 to your magic number 5:

This isn't possible, unfortunately. Since the SimpleInjector.dll is strongly signed, all assemblies that it allows access to its internals, should be strongly signed as well. As far as I know, strongly signing the dynamically created assemblies isn't possible. Or at least, not without leaking the strong name key.

But if you know a way that allows those types to be marked as internal, please say so, since I hate it that those types are part of the public API.

>> Why do you have one dynamic assembly per container? It's more efficent than having only one?

Not sure actually. I've just verified that one dynamic assembly per app domain would do as well.

>> I have replace it with an array in my code and the tests pass. It's a little more efficient

Again an interesting idea. Array lookup could increase lookup speed, but there's one downside here. The number of Registration instances a Lifestyle creates could be very big. This will result in really big arrays. This can take up a lot of extra memory and a lot of memory can mean a lot of cache misses, which is bad for performance. And if the user decides to create one container per request, the number of created Registrations is infinite and will cause a memory leak and OutOfMemoryExceptions. Creating one container per request is of course a bad idea, but this should only result in bad performance; not in a memory leak.

Of course this could be fixed by creating array with limited size and store instances at the index "nextRegistrationId % [array size]", but since that is basically what the .NET Dictionary class does, I don't think it would be worth the trouble to create a custom dictionary class.
Feb 3, 2014 at 8:51 PM
Edited Feb 4, 2014 at 2:57 AM
The number of Registration instances a Lifestyle creates could be very big.
If registrations are not shared between scopes you can use a List instead of an array , assign registration an index ( list.count ) first time they are used and add the instance on the list. But I think they are shared... Bad idea :)
Feb 4, 2014 at 9:05 AM
There will be just a single Registration instance per container registration. So Registration instances are indeed shared between scopes.
Feb 4, 2014 at 9:06 AM
One way I though of to remove the LazyScope and LazyRegistration classes is by generating them as public types in the dynamic assembly, but I'm not sure that this is worth the trouble.
Feb 26, 2014 at 8:16 PM
We just released version 2.5.0-beta2 on NuGet and it contains the optimization we discussed (except your last array indexer look-up suggestion). If there aren't any complaints, you can expect a stable release pretty soon.
Feb 26, 2014 at 11:38 PM
Edited Feb 26, 2014 at 11:50 PM
I see dot_NET_Junkie clearly said he does not want to add a GetInstance<>() at the scope and I am a little bit late for the discussing.

Nevertheless here are my five cents:
  1. I would give plus 1 to geovaly's proposal - I would appreaciate the option to ask a scope directly for an instance. For the WebRequest, WCF-OperationContext, TLS or LCC I would simply keep the existing way to resolve scoped-instances from the container - these have to magic implicit ability to find their static-like context without the user having to specity it explicitly.
  2. For the factory methods and registrations: a) I think Register<T>(Func<Container, T> instanceCreator) could be a good addition (from my user-perspective). Regarding the factories I could even imagine to let the user register a Func<Container, T> which would be automatically decorated to allow an injection into a ctor as Func<T> (e.g. as fallback if no Func<T> was registered directly). At least this would avoid any service-locator smell and even offer a way for scope-lookups if container and scope would share some common base (but I fear that dot_NET_Junkie will freak out because that would ask for a disposable container - something like a fallback-root-scope ;-)).
I personlly understand and to some extend share the demand for explicitely specifying the "scoped singleton" cache - e.g. the instance resolution context. At the core I think I was looking for a GetInstance<T>(IServiceProvider cache) which instructed the container to look for instances first of all in the cache (maybe only for types which were marked with a special lifestyle) - if such a method existed it would be probably possible to roll an own impl of a scope as geovaly requested it (explicitely providing a creation context).

BTW I personally also had a few instances where I was looking for the possibility to have child-containers in order to be able to control which services are exposed to which parts (e.g. only the registrations of a shared-core-container should be visible to MVC, ASP.net, SignalR) - with GetInstance<T>(parent) composite-containers could be realized... I am not sure if child-container is another common anti-pattern - I just can tell that I naively were looking for a way to set up something like this to avoid the need for creating completely independent containers. For example: Currently I have one service with creates a special "child" container for WebAPI and registers some services as singletons that are resolved from a main container... this works but it is a little bit of tedious to register the singletons explicitely...
Feb 27, 2014 at 9:06 AM
Edited Feb 27, 2014 at 9:49 AM
Without TLS context dependent resolution would mean to have InstanceProducer.GetInstance(context) and a complete separate (slow) new path for context != null that would do first-chance lookups at the context for all objects or those with a special lifestyle registration... I am not sure if this would be a good idea.

I will create tonight an experimental CacheRegistrationScope impl that uses TLS to temporarily rigester a cache .. but makes the TLS registration independent of the instances life-times...
container.Register<IXyz, Xyz>(ExplicitCacheLifestyle.Default);

var cache = new InstanceCache();
cache.RegisterSingle<IXyz>(new Xyz());   // allow custom registration of instances in an instance cache

using (new ExplicitCacheScope(container, cache))
{
container.Resolve<IXyz>();         // resolve instances from the temporarily in TLS registered scope
}

// somewhere else

cache.Dispose();
Feb 27, 2014 at 10:57 AM
I understand the requests for having a Register<T>(Func<Container, T>) method and having GetInstance methods on the Scope, but I'm not convinced this is an improvement.

Having GetInstance methods on the Scope class could be possible, but that would effectively make the Scope class into a child-container, which it is now currently not. The Scope is now currently a lifestyle-cache (they work with the Registration classes), while there is an extra layer involved when calling Container.GetInstance methods. That layer is the InstanceProducer class, which will allow registrations to be intercepted/decorated. When users start calling scope.GetInstance<T>, they would expect to get the same instance as when calling container.GetInstance<T> (when the that scope is the currently active scope), but that's currently not how the Scope works. Besides, the Scope is currently designed to cache instances, but what if the user requests a transient instance; that should certainly not be cached. So instead the call should be forwarded to the container and the scope must be passed on. In fact, container.GetInstance<T> does the same as scope.GetInstance<T> except that you would be able to call scope.GetInstance<T> on a scope that is currently not the active scope. Conclusion, calling GetInstance of scope makes it a child container.

Child containers however are a feature that should be hardly ever needed, and can almost always be implemented in a different way. It's because of this that Krzysztof Koźmic years ago questioned whether child container's should been removed from his Castle Windsor framework. And for Simple Injector there are good arguments to have it. 1 Child-containers are slow 2. There are alternatives.

Next point, since Simple Injector doesn't split up building and resolving in two separate types (as Autofac does), passing in a Func<IContainer, T> delegate isn't strictly needed, as it is with Autofac, since the user can always access the container instance directly. That's the initial reason why I chose Func<T> for Simple Injector. Registering "context =>" just gives more noise. Adding an extra Register(Func<IContainer, T>) overload will confuse developers. What to choose, when, and why? So this feature starts at -100 points what I'm concerned, and it has to have very strong benefits to survive.

Besides, it seems registering an Func<IContainer, T> is only needed when dealing with child containers. This is where a call to Scope.GetInstance would forward the call to the container, passing on a reference to itself. With the current scoped lifestyles, passing on the scope is not needed, since the container can always get the current active scope for the object graph that is being resolved.

So from an API perspective, I don't think that adding an Register<T>(Func<IContainer, T>) overload would be a good idea. Internally however, it might still be an option to compile Func<Scope, T> delegates, as @geovaly proposed. That would really allow the Scope (or scopes) to be resolved just once per object graph, which would further improve performance. Although not easy to implement, it is completely possible to implement this without making any changes to the API. So it will be an internal optimization. There are however are enough challenges with this, to make me want to postpone such optimization. One of the challenges is that of multiple scopes. It's possible for the user to have multiple scoped lifestyles in the same object graph and use them at the same time. It's not a common scenario, but currently valid. In other words, the internally compiled delegates should accept all the Scopes they need, e.g. Func<Scope, Scope, Scope, T> in case there are three scopes. But that's just one.
Marked as answer by dot_NET_Junkie on 4/15/2014 at 1:47 AM
Feb 27, 2014 at 9:48 PM
Edited Feb 27, 2014 at 10:17 PM
Hello Steven and blueling ( what is your name? )

The reasons that I like to ask the scope directly for an instance is
  1. Is explicit. You tell the user that the instance scope is his current lifetime. I don't like implicit things. I don't like magic. Magic is hard to understand. ("these have to magic implicit ability to find their static-like context without the user having to specity it explicitly."). So Container.GetInstance will not be the same as scope.GetInstance because each one use different current scope.
  2. You don't need thread static fields. The static field is a singleton and (mutable) singletons are an antipattern.
  3. You can have a good abstraction. You have a hierarchy of scopes. The container is the root scope. When you begin a lifetime scope you create a child scope. So you don't need Func<Scope, Scope, Scope, T>. You need only one scope ( the current scope) because every scope has a parent. (similar with autofac).
Feb 27, 2014 at 11:00 PM
To implement this is a big breaking change. So if in the future you decide to take this path ( a new version or a new project ) please count me in ;)