Resolve method is explicitly implemented, but the readme shows `new Container().Resolve()` about stronginject HOT 21 CLOSED

We need an extension method, so that when there's multiple types you can disambiguate via a type parameter rather than a cast.

Options are to generate an implicit instance method and keep the extension method, or simply to move the extension method to the global namespace.

I suggested always available extension methods for exactly this case, but I doubt that's likely to happen.

from stronginject.

@zejji I'm interested in what your alternative design would be?

I would hope that the runtime would be able to devirtualize these calls, but such things are often hard to rely on. However I would be extremely surprised if there were use cases for StrongInject where this actually made a difference. The expensive part of DI is rarely the interface call to start the whole process off. If you can show a benchmark where that occurs, then I'll definitely take a look at alternative designs.

This is particularly the case as the container is likely to implement a large number of interfaces. In my case, for example, I will be registering many hundreds of CQRS command/query/event handlers.

In the vast majority of cases the container should only have to implement one interface, which kicks off resolution of all registered objects, and stronginjects API is designed to guide you in that direction. There are some exceptions, but I think it's worth checking that you're actually in such a case. Perhaps you'd like to discuss on https://gitter.im/stronginject/community and we can see if there's perhaps a better solution for your use case?

from stronginject.

@YairHalberstadt - Likewise, thanks for the discussion, which was a good chance to think things all the way through! If I get a spare moment, I will try to do the benchmark which I mentioned above and let you know the results.

from stronginject.

If you are using StrongInject there's a Resolve extension method available.

from stronginject.

Oh, I see! The IDE doesn't seem to provide a way to discover this in places where you didn't happen to add the using directive yet. Even typing .Resolve() resulted in an error and no suggestions.

Not sure how this could be improved, other than generating an instance method rather than an extension method. What do you think?

from stronginject.

Given that the extension methods are on StrongInject.IContainer<T> instances only, I'm not thinking of a downside to using the global namespace. This would have an advantage over an instance method because you would see how to disambiguate when a type parameter is required.

from stronginject.

I'm worried that it clutters the global namespace with types. I suppose this can be mitigated by calling the type StrongInjectContainerExtensions, and marking it hidden from IDEs.

from stronginject.

That's a good point though. I personally will probably not be affected either way now that I know that the using directive is important. Perhaps a code comment in the readme right before the using directive could explain that it must be added before Resolve appears in intellisense?

from stronginject.

Using explicit interface implementations rather than standard class methods seems an odd choice from a performance perspective (in a library where one of the principal aims is performance) for the reasons given here:

• https://www.thomasclaudiushuber.com/2020/03/05/c-the-order-of-interfaces-is-important-for-casting-performance/
• dotnet/runtime#7291

This is particularly the case as the container is likely to implement a large number of interfaces. In my case, for example, I will be registering many hundreds of CQRS command/query/event handlers.

from stronginject.

@zejji It sounds like you might be interested in #139. I need to update the code samples to match what StrongInject actually enables now with Func<Owned<T>> instead of IFactory<T> which the examples incorrectly use.

from stronginject.

@YairHalberstadt - My use case is a web application which has the following characteristics:

1. Requests into the system take the form of either commands or queries. These are dispatched by a mediator object to the appropriate handler, e.g.:

// The mediator is a singleton which uses a DI container to resolve services.
Mediator mediator = new(new MyContainer());

// Example GET request
app.MapGet("/weatherforecasts", () =>
{
    var response = mediator.Send(new GetWeatherForecast.Query());
    return Results.Ok(response);
})
.WithName("GetWeatherForecasts");

// Example POST request
app.MapPost("/weatherforecasts", (CreateWeatherForecast.Command command) =>
{
    var response = mediator.Send(command);
    return Results.Ok(response);
})
.WithName("CreateWeatherForecast");

2. My mediator implementation is generated by a custom source generator. In very simple cases it looks like the following (ignoring async here for the sake of simplicity):

public partial class Mediator
{
	private readonly MyContainer _container;
	...
	public GetWeatherForecast.Response Send(GetWeatherForecast.Query query)
	{
		// Resolve the query handler and call its 'Handle' method, passing the query object.
		return _container.Run<IQueryHandler<GetWeatherForecast.Query, GetWeatherForecast.Response>, GetWeatherForecast.Response, GetWeatherForecast.Query>((handler, query) => handler.Handle(query), query);
	}
	...
}

In more complicated cases, the mediator also supports pipeline behaviors and one or more event notification handlers, but this does not affect the current discussion.

3. Because of this structure, which I believe is fairly common, I do not see how it will be possible for me to use a DI container which only implements one interface: in practice a large application is likely to require the container to implement many hundreds of interfaces. I also do not see why StrongInject could not use standard method calls rather than explicit interface implementations.

(In my own mediator implementation, the interface is also generated by the source generator from attributes placed on the mediator's partial class definition, meaning that there will only ever be one interface for the mediator, which includes "Send" methods for all command/query/event types), but I understand that this is not the approach taken by StrongInject.)

Explicit interface implementation is only required if a class implements two or more interfaces that contain a member with the same signature (since otherwise a single member on the class will be used as the implementation). See e.g. here. However, when using a DI container, one generally only wants to register a single implementation, or a collection of implementations, against one particular type.

So my question would be - in the most common scenarios, why is explicit interface implementation required for StrongInject? There does not appear to be any ambiguity which requires it. I agree that the performance impact is likely to be small in practice, but it appears entirely unnecessary, as well as potentially causing confusion. This is evidenced by the existence of this thread, which I also came across because I did not realize it was necessary to add the using StrongInject; statement to get the extension methods (as I did not see that they would be required).

For reference, here is a very brief example which can be pasted into LINQPad containing a container implementation which uses standard member functions rather than explicit interface implementations:

void Main()
{
	Container myContainer = new();

	// Example call to Container.Run() using explicit typing (not required in this case).
	// Note: MyHandler1 has been registered with the DI container as the implementation of IQueryHandler<MyQuery1, MyResponse1>
	var response1 = myContainer.Run(
		func: (IQueryHandler<MyQuery1, MyResponse1> handler, MyQuery1 query) => handler.Handle(query),
		param: new MyQuery1("What is the answer?"));

	// Example call to Container.Run() using implicit typing.
	var response2 = myContainer.Run(
		func: (handler, query) => handler.Handle(query),
		param: new MyQuery2("This is another query"));

	response1.Dump(nameof(response1));
	response2.Dump(nameof(response2));
}

// Query handler interface
public interface IQueryHandler<TQuery, TResponse>
{
	TResponse Handle(TQuery query);
}

// Queries
public record MyQuery1(string Question);
public record MyQuery2(string AnotherQuestion);

// Handlers
public class MyHandler1 : IQueryHandler<MyQuery1, MyResponse1>
{
	public MyResponse1 Handle(MyQuery1 query)
	{
		return new MyResponse1("42");
	}
}

public class MyHandler2 : IQueryHandler<MyQuery2, MyResponse2>
{
	public MyResponse2 Handle(MyQuery2 query)
	{
		return new MyResponse2("The answer is always 42");
	}
}

// Responses
public record MyResponse1(string Answer);
public record MyResponse2(string AnotherAnswer);

// Container
public class Container
{
	public MyResponse1 Run(Func<IQueryHandler<MyQuery1, MyResponse1>, MyQuery1, MyResponse1> func, MyQuery1 param)
	{
		// This body is for demo purposes only - a real implementation would handle resource disposal correctly. We are only interested in the method signature here.
		var dependency = new MyHandler1();
		return func(dependency, param);
		
	}

	public MyResponse2 Run(Func<IQueryHandler<MyQuery2, MyResponse2>, MyQuery2, MyResponse2> func, MyQuery2 param)
	{
		var dependency = new MyHandler2();
		return func(dependency, param);
	}
}

from stronginject.

@zejji thank you for your detailed explanation. Much appreciated!

I don't understand something.

You said:

Explicit interface implementation is only required if a class implements two or more interfaces that contain a member with the same signature (since otherwise a single member on the class will be used as the implementation). See e.g. here. However, when using a DI container, one generally only wants to register a single implementation, or a collection of implementations, against one particular type.

However you also say that in your case your container will implement hundreds of IContainer interfaces, so it seems to fit this case (at least for the Resolve method if not for the Run method).

I think the reason I chose to do it this way was it was the easiest way to make sure I was generating valid code where the methods weren't illegal overloads of each other for whatever reason.

I'm definitely happy to implement the method implicitly when there's exactly one interface. I'll have to think about whether there's any cases with 2 interfaces where doing so could be illegal.

I'm also happy to move the ContainerExtensions into the top level namespace if both you and @jnm2 found their discoverability too low.

from stronginject.

I'm definitely happy to implement the method implicitly when there's exactly one interface.

By the way this might not be as helpful as you'd like. Far more problematic for performance than interface calls are allocations, and so the container interface looks like this:

TResult Run<TResult, TParam>(Func<T, TParam, TResult> func, TParam param)

This allows hardcore performance fanatics to call a func which requires state without capturing it, by passing it in a struct to TParam.

Extension methods convert that to more friendly but less performant methods, such as

public static TResult Run<T, TResult>(this IContainer<T> container, Func<T, TResult> func);
public static void Run<T>(this IContainer<T> container, Action<T> action);

So unless you want to call the 'hardcore' version, implicit implementations won't actually help you.

from stronginject.

@YairHalberstadt - Thanks for your response.

Re the Resolve method - yes I agree that an extension method is unavoidable here, since method overloads cannot differ only by return type, which is one reason why my inclination would be to generate a single IContainer interface from attributes rather than requiring the user to add a separate interface per type. However, I realize that would be a significant API change at this stage (and therefore probably not a realistic possibility).

from stronginject.

my inclination would be to generate a single IContainer interface from attributes rather than requiring the user to add a separate interface per type

Can you give an example of what you mean by this?

from stronginject.

@YairHalberstadt - just spitballing, but an implementation for Resolve which does not use explicitly implemented interfaces would look something like the following. However, the (arguable) benefit of removing the very small performance cost of casting to an interface results in a slight additional maintenance cost - namely, the inability to rename a registered type without also having to rename calls to the corresponding Resolve method. And some of the Resolve method names could get quite ugly in the case of generic interfaces taking multiple type parameters...

So I can see why you have taken the approach you have taken (in order to ensure a uniform interface). Sometimes I miss C++ template specialization (although not that much) 😛

It would be interesting to run a benchmark with a few tens (or hundreds) of thousands of handlers to see whether the performance impact would ever actually be measurable in a very large monolithic application. Like you say, recent improvements in C#'s interface casting performance may make this issue moot.

void Main()
{
	Container myContainer = new();
	
	var myIInterface1 = myContainer.ResolveIInterface1();
	var myIInterface2 = myContainer.ResolveIInterface2();
	
	myIInterface1.DoSomething();
	myIInterface2.DoSomethingElse();
}

public interface IInterface1
{
	void DoSomething();
}

public class MyClass1 : IInterface1
{
	public void DoSomething()
	{
		Console.WriteLine($"Hello from {nameof(MyClass1)}!");
	}
}

public interface IInterface2
{
	void DoSomethingElse();
}

public class MyClass2 : IInterface2
{
	public void DoSomethingElse()
	{
		Console.WriteLine($"Hello from {nameof(MyClass2)}!");
	}
}

public interface IContainer
{
	IInterface1 ResolveIInterface1();
	IInterface2 ResolveIInterface2();
}

public class Container : IContainer
{	
	public IInterface1 ResolveIInterface1()
	{
		return new MyClass1();
	}

	public IInterface2 ResolveIInterface2()
	{
		return new MyClass2();
	}
}

from stronginject.

@zejji but how would we know to generate ResolveIInterface1 and ResolveIInterface2?

Implementing a method for every single registered type is bad for a number of reasons:

1. There may be hundreds or even thousands of registered types. Generating so much code which will never get used is definitely bad for performance!
3. We will have to give warnings/errors for issues when resolving these generated types even when they're never used.
   For example:

interface IService
record A() : IService
record B() : IService
record C(IService[] services)
[Register(typeof(A), typeof(IService))]
[Register(typeof(B), typeof(IService))]
[Register(typeof(C)]
class Container{}

Now what I actually want to resolve is C, which is perfectly ok. But the container will try to generate code to resolve IService as well, and will have to error/warn because there's multiple implementations registered for IService.

c) You might not have registered the type you actually want to resolve - e.g. Func<C>. For any given registration there's an infinite number of possible ways it can be resolved. As Func<C>, C[], Owned<C>, Func<Owned<Func<C[]>>` etc.

from stronginject.

@YairHalberstadt - Again, just thinking out loud:

1. You could have two separate attributes (or an attribute argument) which would indicate whether to generate a Resolve method for the type or not. This would prevent unnecessary code being generated.
2. If you wanted to ensure that changing the type name did not break code, you could also have an optional explicit name for the generated Resolve method (passed via an attribute argument). This would be a similar idea to named routes, although in practice thinking of good names could be quite hard!

from stronginject.

@zejji whilst all of that is possible, I think the usability difficulties for the majority of users who aren't impacted by the performance impacts is significant enough that the benefits are not worth the costs. Thanks for the ideas though!

from stronginject.

Closing now that the extensions have been moved to the global namespace.

If it's possible to show the performance impact of the interface calls in a realistic scenario, I'll consider making them implicit, but we can discuss in a future issue.

from stronginject.

@YairHalberstadt - As promised, I've created a benchmark using BenchmarkDotNet which is intended to help identify whether there is any significant cost to using a large number of interfaces and explicit interface implementations for the container's 'Run' methods, rather than standard class methods.

As mentioned above, I was interested to know the performance impact with a view to using StrongInject for a CQRS-based application with a large number of command, query and event handlers. There will be separate registered handler class for every action that occurs in the system, leading to potentially thousands of handlers that need to be resolved directly from the container using the corresponding 'Run' method. Obviously, each additional 'Run' method requires the DI container to implement the generic IContainer interface for the resolved type, resulting in a container class that implements thousands of interfaces.

The repo can be found here.

The project is a console app which generates source code for two container implementations, one using explicit interface implementations for each 'Run' method, and one using standard class methods. It then benchmarks these two implementations by calling each 'Run' method on both containers in a random order. The same order is used for both containers. The method which is called on each service resolved from the container is a trivial method which performs some addition operations.

As for the results, unless there is something obviously wrong with my benchmark (which there may well be :p), it doesn't look like there is a meaningful overhead to using interfaces. The difference starts to become significantly more noticeable with 5000 methods, but even so the version with interfaces is still not an order of magnitude slower (i.e. it is between 4 and 5 times slower, which is unlikely to be significant when the method body actually performs real work as opposed to a few additions).

Here are the results from a few test runs:

Containers implement 1000 'Run' methods; each method is called in random order:

// * Summary *

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
Intel Core i7-10610U CPU 1.80GHz, 1 CPU, 8 logical and 4 physical cores
.NET SDK=6.0.100
  [Host] : .NET 6.0.0 (6.0.21.52210), X64 RyuJIT

Job=InProcess  Toolchain=InProcessEmitToolchain  IterationCount=15
LaunchCount=2  WarmupCount=10

|                  Method |      Mean |    Error |   StdDev |   Gen 0 | Allocated |
|------------------------ |----------:|---------:|---------:|--------:|----------:|
|    NoInterfaceBenchmark |  42.75 us | 0.335 us | 0.481 us | 11.4746 |     47 KB |
| WithInterfacesBenchmark | 105.22 us | 1.801 us | 2.525 us | 11.4746 |     47 KB |

Containers implement 5000 'Run' methods; each method is called in random order:

// * Summary *

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
Intel Core i7-10610U CPU 1.80GHz, 1 CPU, 8 logical and 4 physical cores
.NET SDK=6.0.100
  [Host] : .NET 6.0.0 (6.0.21.52210), X64 RyuJIT

Job=InProcess  Toolchain=InProcessEmitToolchain  IterationCount=15
LaunchCount=2  WarmupCount=10

|                  Method |       Mean |     Error |    StdDev |     Median |   Gen 0 | Allocated |
|------------------------ |-----------:|----------:|----------:|-----------:|--------:|----------:|
|    NoInterfaceBenchmark |   388.0 us |  25.38 us |  37.99 us |   407.5 us | 57.1289 |    234 KB |
| WithInterfacesBenchmark | 1,499.6 us | 324.22 us | 475.24 us | 1,108.9 us | 56.6406 |    234 KB |

from stronginject.