Dispose services in reverse creation order #505
Conversation
| resolvedExpression); | ||
| resolvedVariable); | ||
|
|
||
| Expression captureDisposableExpression = Expression.Invoke( |
There was a problem hiding this comment.
Errr? Give me the summary of this fix? You're now capturing all disposable objects in the same list? It's no longer transient disposables and this point right? We originally removed this because of memory concerns.
There was a problem hiding this comment.
We never tested the actual performance in terms of op/s, just memory allocations. If you look closely at your original PR you'll notice that the memory allocations measured are mostly related ServiceProvider.CreateScope()/ctor, and there are no measurements related to the actual capturing disposables of any kind.
If you're really concerned memory you could store some sort of ordering metadata with the captured services (a la Autofac) or pool the Dictionary/List like @benaadams suggested in the original PR. Both sound kinda complicated and might even hurt perf in some scenarios.
It might be a good idea to add a BenchmarkDotNet scenario that covers capturing disposables.
There was a problem hiding this comment.
Yes, we need measurements. Also ConcurrentBag did get way less allocatey in .NET Core 2.0 so maybe it's worth a remeasure.
There was a problem hiding this comment.
This change would only add scoped and singletons to that list, there aren't many singletons and scope objects are short-lived so I am not super worried about this.
There was a problem hiding this comment.
Also, all of this work to elide calls to CaptureDisposable this should be repeated here https://github.com/aspnet/DependencyInjection/blob/dev/src/Microsoft.Extensions.DependencyInjection/ServiceLookup/CallSiteExpressionBuilder.cs#L150.
| serviceCollection.AddSingleton<IFakeMultipleService, FakeDisposableCallbackInnerService>(); | ||
| serviceCollection.AddSingleton<IFakeMultipleService, FakeDisposableCallbackInnerService>(); | ||
| serviceCollection.AddSingleton<IFakeMultipleService, FakeDisposableCallbackInnerService>(); | ||
| serviceCollection.AddSingleton<IFakeService, FakeDisposableCallbackInnerService>(); |
There was a problem hiding this comment.
Nit: might make sense to have a test that mixes service lifetimes. Could be relevant if we go back to tracking disposables using multiple collections.
|
|
||
| namespace Microsoft.Extensions.DependencyInjection.Specification.Fakes | ||
| { | ||
| public class FakeDisposeCallback |
There was a problem hiding this comment.
There's already 1000 .cs files in Specification.Tests/Fakes, just put all the public classes in their own file.
| Expression.Block( | ||
| assignExpression, | ||
| addValueExpression, | ||
| captureDisposableExpression)), |
There was a problem hiding this comment.
Suuuper nit: match ordering with runtime resolver. I know it doesn't really matter.
There was a problem hiding this comment.
A benchmark would be nice to alleviate @davidfowl's concerns.
pakrym
force-pushed
the
pakrym/dispose-order
branch
from
170848f
to
000fff9
Compare
Eliminate redundant CaptureDisposable call from expression trees Split fakes into separate files
@davidfowl