Fix #948 make component registry immutable #981
Conversation
…vices into a dedicated tracker
…istryBuilder Fix test because adding adaptees can only be done in a lifetimescope
Beginning a lifetime scope without changing the underlying registry doesn't require this construction.
|
If this PR is going to be approved, I'll update the documentation as well. NotesI have assumed that this PR will be merged into a version where
|
|
Thanks for this! It will take a while to get through since a lot has changed, and it'll also require some time to think through all the use cases. I'm excited to dive in and see how you solved one of the larger challenges on the backlog. I'd like to make sure @alexmg has a chance to check it out, too, before any final decision gets made. Just to set up expectations that it probably won't happen, like, tomorrow or something. Appreciated, not forgotten, just not immediate. Yay for "real life" work taking over, right? ;) |
…rvicesTrackerTests'
|
I haven't forgotten! I've pulled the PR down to my local machine and I'm looking at it now. For the most part, I dig it. I'm trying to mess around with the changes from a consumer perspective and see what's different, running the benchmarks to see how perf is affected, if at all. That's actually where I've gotten stuck. The I'll have to work through that so I can see what the changes mean as far as performance. It may require some additional benchmarks to exercise areas like lifetime scope creation and resolution from scopes. Most of the benchmarks currently resolve directly from a container, which is good to exercise certain subsystems (like decorators) but it doesn't give us insight into more hierarchical systems that are more real-world. Not to say that's a task for this PR, just keeping things up to date. If I update the benchmarks in the develop branch, I may do a pull from that branch into this PR to keep things apples-to-apples. That'd also resolve some of the recent merge conflicts due to me moving things around in the unit tests. :) |
|
Sure, no problem. I'll just check in once in a while. |
|
OK, here's what I did:
What this effectively does is "take the PR" but allow us to test some things like adding more benchmarks without holding this up or leaving it open forever. It also lets us target it for a 5.0 release and plan around when it should be brought into develop without holding up any minor changes that need to happen first. Likely this will end up being the "breaking changes for 5.0" branch. ;) This is definitely one of the largest and most sweeping changes I've seen come in a PR, and I can't overstate how much I appreciate the work that happened here. I like what I see and I think if anything needs to change we can do whatever minor massaging needs to happen right in the branch. So... thanks! 🎉 |
| /// <summary> | ||
| /// Protects instance variables from concurrent access. | ||
| /// </summary> | ||
| private readonly object _synchRoot = new object(); |
There was a problem hiding this comment.
Really amazing, first-class PR :-) !!! 👏 🎉
Just one (late) question - does this class need synchronization now? The only times registry builders are mutated are during initial container build-up (required to be single-threaded) and during scope creation (also single-threaded).
There was a problem hiding this comment.
Thank you :-).
Now that I look at this again, I think you are right and this can be removed. I don't remember why I left it in, or if I actually had a reason to leave it in.
There was a problem hiding this comment.
Sounds great! This is all in the pr-immutable-container branch. I still haven't had a chance to really dig in due to some personal stuff coming up unexpectedly.
| /// An <see cref="IDictionary{TKey, TValue}"/> that can be used to share | ||
| /// context across registrations. | ||
| /// </value> | ||
| public IDictionary<string, object> Properties { get; } |
There was a problem hiding this comment.
For example, regarding synchronization, Properties here will allow the underlying Dictionary<,> to be accessed outside the _syncRoot lock, which is potentially non-threadsafe.
(I'm happy to attempt a PR for these things but just thought I should confirm my understanding first.)
There was a problem hiding this comment.
That is true. I kept this as it is now. This is something that could be improved.
|
Great PR @weelink. Thanks for taking the time to put this together. I have a branch with changes to get the |
|
Would be a nice target for the Is there a plan to introduce new optimisations now that component registries are immutable? (Intrigued where all this is going to land - lots of possibilities :-)) |
|
@weelink I just wanted to let you know this has now been incorporated into the |
Make
IComponentRegsitryimmutable by moving all mutable actions toIComponentRegistryBuilder.