-
-
Notifications
You must be signed in to change notification settings - Fork 839
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
WIP: Pipelines #1121
WIP: Pipelines #1121
Conversation
Tests are passing; refactoring and additional tests needed. Also some performance investigation needs to happen; I'm seeing about a 300ns additional cost on all resolves. I'll take a look at it. |
Had to rework some stuff, including some improvements in
A new thing I realised; every time you reference a method (instance of static), it creates a new
That compiles to:
So I changed it to use a Func instance created in the constructor to save the additional allocation memory and time. Child Scope resolve is still a bit problematic:
That's because a pipeline needs to be built in each nested lifetime scope for any dependencies bought in via the ExternalRegistrySource. I'm going to have to see if there's a way to reference the outer component pipeline without re-building it. Before I do that though I'm going to get everything ready for the first review. |
This is exciting stuff! I've been loosely following as the push notifications come through and to see you have this working to within a pretty reasonable set of tolerances to the original is sweet. I'll pull it down local and try it out when it's ready. From a design perspective, have you noticed anything interesting in comparing/contrasting with the original design about how things are... easier/harder to work with? more/less extensible? |
The design makes it much easier to insert things 'in the middle', as it where. For example, attaching something that runs before circular dependency detection, or between sharing return and decorator lookup. The pipeline mechanism also makes it easy to 'bail' at any point (for example, in the sharing middleware) and not continue down the path. The extensibility is great; should make it easier to do some of the more interesting stuff that we would like to do in the integrations. Personally I find the execution path slightly easier to parse in my head now, but that might be because I wrote it. There are some additional risks with the added flexibility this grants you; technically users could insert a pipeline stage that really breaks things. There's a couple of mitigations to that:
One exciting thing, I don't know if you've seen, but early traceability is already in there (I had to add it to figure out a problem I was having...). For example, the following code: var builder = new ContainerBuilder();
builder.RegisterType<A>().InstancePerLifetimeScope();
var container = builder.Build();
var lifetime = container.BeginLifetimeScope();
// Create a tracer.
var tracer = new DefaultDiagnosticTracer();
// Get notified when operation trace content is ready.
tracer.OperationCompleted += (sender, args) =>
{
Console.WriteLine(args.TraceContent);
};
// Use the tracer.
lifetime.TraceWith(tracer);
var ctxA = lifetime.Resolve<A>();
var ctxA2 = lifetime.Resolve<A>(); Tracing is inherited between lifetime scopes; so if you add tracing at the container level, all scopes get it, but you can apply it per-scope as I've done here. This gives you some output:
Note that the second request ended at the sharing stage. |
Wow, that's so cool! I saw there was some tracing but since I hadn't really executed anything with it yet I didn't entirely grasp what all was going on. Nice! |
Yeah, I'm fairly pleased with it; people can write their own tracers if they want, and I can imagine the benefits of adding things like a I've got some renames and tidy-up to do, but the whole thing should be ready for consumption in a day or two. |
Squashed commit of previous related work because there was a lot of discarded changes early on that won't make sense.
Ok; I'm happy with naming, I've squashed most of the commits for pipelines, because the early ones were off base and would have caused confusion. Any subsequent changes (as an output of review) will not be squashed. This is ready for consumption now; I'm going to take this out of draft, then I'll write some more words in here. |
Slight addendum; there are still some additional tests I need to add by the way, but thought I'd get it ready for people to look at. There are also additional changes for pipelines that I'd like to make before the v6 release (namely around how decorators work). I wanted to get the first batch of changes signed off though before I do that. |
PipelinesI'm going to have a go at describing how the new pipelines functionality works here. ConceptsA 'Resolve Pipeline' is made up of a sequence of middleware. Each middleware item defines:
The execute method takes a context object for the request, and a callback to invoke that proceeds to the 'next' piece of middleware. Here's what a piece of middleware looks like: /// <summary>
/// A middleware example.
/// </summary>
internal class MyCustomMiddleware : IResolveMiddleware
{
public PipelinePhase Phase => PipelinePhase.Activation;
public void Execute(IResolveRequestContext context, Action<IResolveRequestContext> next)
{
// Do something before subsequent pipeline stages.
// Manipulate the context and access content.
// Some sample properties:
// - context.Registration
// - context.Parameters
// - more...
try
{
// context.Instance will be null
next(context);
// context.Instance should be populated now
}
catch(Exception ex)
{
// Handle errors from later in the pipeline.
}
finally
{
// Do stuff whether the pipeline starts or fails.
}
}
} When all the middleware in the pipeline has been executed, the Registration and Pipeline BuildingPipeline ConstructionDuring registration, there is a At registration time, prior to var builder = new ContainerBuilder();
var registration = builder.Register(ctxt => new A()).As<IA>();
// Instance middleware.
registration.ResolvePipeline.Use(new MyCustomMiddleware());
// Delegate middleware.
registration.ResolvePipeline.Use(PipelinePhase.ParameterSelection, (ctxt, next) =>
{
// Do your middleware stuff.
next(ctxt);
});
var container = registration.Build(); The existing Built-in MiddlewareThe 'default' Autofac behaviour, around sharing, activation, etc, is all added last, when the user calls This is done for two reasons:
When the container This method does the following:
Activators have a slightly special case, because of changes I think it would be good to have in the future. Instead, it has a This will enable activators to make choices about what stages to add at container build time, Check out the updated Dynamic RegistrationsDynamic registrations (from registration sources at execution time) also build the component pipeline in the same way, only instead of building pipelines when the container is built, they Generating the Concrete PipelineCalling When building the pipeline, the last middleware in the pipeline receives a special 'terminating' action as its I think this mechanism is going to be used to be able to define a service-specific pipeline that will continue executing a registration's pipeline inside the scope of the service-specific middleware. Decorators may end up being like this before 6.0 goes out. ResolvingThe resolve process is probably the easy bit! The
The resolve operation does not do any circular dependency detection. This is now done inside The In addition, to handle scenarios that need to resolve a service while using a completely different circular dependency stack (decorators need to do this), there is a Speaking of tracing.... TraceabilityOne of the great features that have come out of this pipelines work is the ability to trace resolve operations. For diagnostics it's going to be super useful, It's easy to add our default tracing: var builder = new ContainerBuilder();
builder.RegisterType<ImplementorA>().As<IDecoratedService>();
builder.RegisterDecorator<DecoratorA, IDecoratedService>();
var container = builder.Build();
container.AttachTrace((req, trace) =>
{
Console.WriteLine(trace);
});
var factory = container.Resolve<Func<IDecoratedService>>();
var decoratedService = factory(); That will give us two trace events, with the following content (one for the Func resolve and one when the factory method is invoked):
Pretty cool, there's plenty of detail included. There's also error handling in the tracer; here's what you get for a circular dependency error:
Note that it distinguishes the failing middleware stages, indents the exception and all that. Custom TracingSomeone can implement their own tracer by implementing |
Astonishing work! :) The ASP.NET Core Pipeline works very similar, if I am not mistaking, right? |
Thanks! Pretty much, yeah, except the ASP.NET Core pipeline is fully async, whereas this is not. I picked the 'Use' method name to line up with ASP.NET Core middleware terms. |
I'll do what I can to check this out and play with it in the next few days. I'm pretty excited about the potential to solve some of the tracing and circular dependency challenges we've seen in the past. #788 could be solved with some of this, for example. Or integration with a profiler UI. Hooking into stuff like DiagnosticSource might also be interesting. |
Let me know if you have questions. There should be lots of tracing options available to us now. I feel like a full-on profiler UI might be above-and-beyond, but a generated report might be handy, that includes analysis results and suggestions? Like the Autofac.Analysis library by @nblumhardt that followed that profiler UI. Not sure how much we want to ship Autofac with, and how much we ship separately. The DiagnosticSource library is intended for production use, right? I guess if we're not building big trace strings, or not tracing every middleware, that might be fine? Just wary of adding time to the resolve operation. |
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.
I'm still working through here but wanted to get some fast feedback out. I've been trying to stick dotTrace and dotMemory on it to see if I can find any obvious things, but the way BenchmarkDotNet spawns off a different process isn't really profiler-friendly. If you have ideas on how to get that to happen without just writing a separate app that mimics the benchmarks in a single-process console app... I'm open.
src/Autofac/Core/Resolving/Pipeline/IPipelineResolveOperation.cs
Outdated
Show resolved
Hide resolved
Regarding the benchmarks, you can run BenchmarkDotNet with ETW capture enabled, and it will capture traces during the actual run (pass For my trace attaching I did what you said, added a little console app that ran a few warmup steps, then ran a big loop in a separate thread so I could isolate it in dotTrace. Seemed to work ok for me (though obviously not as scientific as the benchmarks). static void Main(string[] args)
{
var bench = new ChildScopeResolveBenchmark();
bench.Resolve();
bench.Resolve();
Task.Run(() =>
{
for (int idx = 0; idx < 10000; idx++)
{
bench.Resolve();
}
}).Wait();
} |
It confuses matters when multiple objects in a middleware function as slighty different component contexts.
I'm working on a second pass review, hitting things with a profiler. I noticed
That might reduce some allocations and speed things up just a little. Doesn't have to be part of this PR, could be a separate issue for later. |
Yes, there's definitely things we can do with the constructor bindings to optimise them. An additional optimisation is that if you have multiple constructors, that have overlapping service types, currently each of those constructors will go and check for a registration for each constructor/type combo. It may be more efficient to determine a set of unique type dependencies at pipeline build time, determine which of the type dependencies we can satisfy at resolve time, and pick the constructor that satisfies the most dependencies. As you say, we can do this in a subsequent PR. |
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.
OK, I think what I'm starting to find are little microperf things, where we could improve tiny places and possibly get an overall improvement, but none of that is enough to stop the PR from going through. I'm not finding anything super obvious to change or improve - this is really good stuff. Let me know what you think about the remaining comments and I think we can call it good.
src/Autofac/Core/Resolving/Middleware/CircularDependencyDetectorMiddleware.cs
Show resolved
Hide resolved
A note on branches; I've currently got this targeting the @tillig, if you think our next release will be 6.0 I can retarget to develop, or we can leave it in v6 for now. I've got changes to how decorators work in the pipelines world still planned, plus it would be good to get composite support (#970) in and resolve some other long-standing issues we can resolve now, so the 6.0 date might be a way off. |
I'm thinking we should keep the v6 branch a little longer to leave room for interim bug fixes that may need to happen as we get 6.0 ready to go. I am pretty wary of long-lived branches, though, since it's so hard to keep them up to date. I noted down four things that I think we need to dive a little deeper on - let me know if these sound right:
Those can all be separate issues that happen after this PR. (Or one "checklist" sort of issue for pre-v6 release.) This is pretty sweet. I'm stoked to see some pretty big overhauls to the Autofac internals that will help us maintain it and take it forward. Great work! 🏆 If you're happy with it, let's merge this bad boy into the v6 branch. We can probably set up a milestone for v6 to tag issues specific to that branch. Might be good to have some sort of master tracking issue to count down to v6 and determine when we think it's ready. |
Yep, good to go, I'll press the big button. 😊 The items you mentioned look good, but I'll add:
I can raise issues for each of those to track them. There's already a v6 milestone, I'll tag this PR, and we can associate anything else of relevance to this. |
Just catching up on the activity after emerging from months of quarantine and home schooling. I've just read through the thread and it's really exciting to see this come together. Amazing work @alistairjevans. I will pull the branch down as soon as possible and take a closer look. |
I've started very early stuff on using pipelines internally, for #1096. Thought I'd raise a WIP pull request to give some visibility of what's happening in my fork.
You can expect plenty of churn as I go...