-
-
Notifications
You must be signed in to change notification settings - Fork 837
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
Fixes #1113- Adding multi-id qualification to stored instances in lifetime scope #1117
Conversation
-- Allows for multiple decorator instances to be shared correctly by storing the instance by its registration ID + the registration ID of its target instance.
@@ -48,7 +48,7 @@ public class LifetimeScope : Disposable, ISharingLifetimeScope, IServiceProvider | |||
/// Protects shared instances from concurrent access. Other members and the base class are threadsafe. | |||
/// </summary> | |||
private readonly object _synchRoot = new object(); | |||
private readonly ConcurrentDictionary<Guid, object> _sharedInstances = new ConcurrentDictionary<Guid, object>(); | |||
private readonly ConcurrentDictionary<(Guid, Guid?), object> _sharedInstances = new ConcurrentDictionary<(Guid, Guid?), object>(); |
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.
Made the latter value nullable, since defaulting to Guid.Empty had a negative impact to a TON of unit tests, triggering a condition that shouldn't have been triggered around self-registration.
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 wondering, if the additional 300ns perf cost is down to the HashCode.Combine and additional equivalence checks in the dictionary, does it make sense to have two ConcurrentDictionary instances? One with a single-guid key, one with a double? Use the single if the qualifying ID is null? That might remove the additional cost for any non-decorator paths.
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 thought the same thing, mostly in the instantiation of the struct plus the more complicated hashcode generation, since I think Dictionary<> uses the hashcode for equivalence on keys. The split sounds like a good path for performance, but does introduce a bit of extra maintenance complexity that could grow until the point that the pipeline stuff comes to fruition. Performance wins out, in this scenario, though!
@@ -48,7 +48,7 @@ public class LifetimeScope : Disposable, ISharingLifetimeScope, IServiceProvider | |||
/// Protects shared instances from concurrent access. Other members and the base class are threadsafe. | |||
/// </summary> | |||
private readonly object _synchRoot = new object(); | |||
private readonly ConcurrentDictionary<Guid, object> _sharedInstances = new ConcurrentDictionary<Guid, object>(); | |||
private readonly ConcurrentDictionary<(Guid, Guid?), object> _sharedInstances = new ConcurrentDictionary<(Guid, Guid?), object>(); |
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 wondering, if the additional 300ns perf cost is down to the HashCode.Combine and additional equivalence checks in the dictionary, does it make sense to have two ConcurrentDictionary instances? One with a single-guid key, one with a double? Use the single if the qualifying ID is null? That might remove the additional cost for any non-decorator paths.
- Improved doc comments for new overloads on ISharingLifetimeScope - Split instance caching into two ConcurrentDictionary collections for single and "qualified" instances, to keep non-decorator paths from seeing performance impact
Benchmarks after Feedback: Expand
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=3.1.200
[Host] : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
DefaultJob : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
|
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.
Looks good! 🚀
Sweet, this looks pretty good now; at least the extra cost (albeit not a lot) is kept to the decorator path. Now my thoughts turn to how breaking this change is (we aren't planning to release 6.0 for a while, it'd be nice if we could make this a 5.x release). The only break here is the addition of two new overloads to ISharingLifetimeScope. I considered using default interface implementations, but we can't use those until we drop support for netstandard2.0 and net461. The number of people that have actually created their own implementation of @tillig, can I get your thoughts on this? My gut says this is a candidate for a 5.x? |
It seems that Hopefully this isn't something that gets pushed until 6.0, since I could really use this and the other fix I put in for my project at work! 🤞 Otherwise, I'll have to hide all of the new decorator goodies since they create blocking issues for us. |
@alistairjevans Agreed, I can't imagine anyone has actually implemented their own I'd be OK sneaking this in as a 5.x release. Maybe a 0.1.0 increment just in case, rather than just a 0.0.1 bugfix. |
Project file and the appveyor.yml. (Wish those would just keep in sync, but... I mean, if someone knows how....) |
Do it appveyor -> csproj? https://www.appveyor.com/docs/build-configuration/#net-core-csproj-files-patching |
Oh, yeah. @VonOgre don't feel responsible for kicking our build around unless you really feel like it. I hate scope creep. I can tackle that as part of the release. |
I'll just go ahead and merge this then, thanks @VonOgre, we'll sort out the versioning. |
Cool. I can do the versioning and release thing shortly. |
@tillig @alistairjevans - Thanks to you both! |
Fixes #1113
As noted in the investigation of #1113, the fact that shared instances are only stored with the registration ID results in some bad behavior when multiple services end up being decorated via Resolve<IEnumerable> and some of the targets are shared.
After trying a fix in #1114, the feedback pointed out the original approach was too expensive for such a commonly executed path. This PR uses the approach that was suggested in the original PR with the below benchmarks. It seems that this change does cost an additional ~300ns per lookup/resolution performed.
Original
Expand
Autofac.Benchmarks.ChildScopeResolveBenchmark-report-github.md
Autofac.Benchmarks.ConcurrencyBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessNestedSharedInstanceLambdaBenchmark-report-github.md
Autofac.Benchmarks.PropertyInjectionBenchmark-report-github.md
Autofac.Benchmarks.OpenGenericBenchmark-report-github.md
Autofac.Benchmarks.DeepGraphResolveBenchmark-report-github.md
Autofac.Benchmarks.ConcurrencyBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessSimpleLambdaBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessNestedLambdaBenchmark-report-github.md
Autofac.Benchmarks.RootContainerResolveBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessNestedSharedInstanceBenchmark-report-github.md
Autofac.Benchmarks.ConcurrencyNestedScopeBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeyedNestedBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessNestedBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessSimpleBenchmark-report-github.md
Autofac.Benchmarks.ChildScopeResolveBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessGenericBenchmark-report-github.md
Autofac.Benchmarks.EnumerableResolveBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessSimpleSharedInstanceBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessSimpleSharedInstanceLambdaBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeyedGenericBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeyedSimpleBenchmark-report-github.md
Post-Change
Expand
Autofac.Benchmarks.ChildScopeResolveBenchmark-report-github.md
Autofac.Benchmarks.ConcurrencyBenchmark-report-github.md
Autofac.Benchmarks.ConcurrencyNestedScopeBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeyedGenericBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeyedNestedBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeyedSimpleBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessGenericBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessNestedBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessNestedLambdaBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessNestedSharedInstanceBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessNestedSharedInstanceLambdaBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessSimpleBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessSimpleLambdaBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessSimpleSharedInstanceBenchmark-report-github.md
Autofac.Benchmarks.Decorators.KeylessSimpleSharedInstanceLambdaBenchmark-report-github.md
Autofac.Benchmarks.DeepGraphResolveBenchmark-report-github.md
Autofac.Benchmarks.EnumerableResolveBenchmark-report-github.md
Autofac.Benchmarks.OpenGenericBenchmark-report-github.md
Autofac.Benchmarks.PropertyInjectionBenchmark-report-github.md
Autofac.Benchmarks.RootContainerResolveBenchmark-report-github.md