[Design]: Changes to improve performance #291
Conversation
- Track disposables in a list with lock instead of a concurrent bag - Allocate a single _createServiceAccessor instead of one per instance - Removed syncObject and just locked the _resolvedServices
There was a problem hiding this comment.
Nobody disposes the root scope, an argument can be made that it is possible but the interface for the top level (IServiceProvider) requires a downcast to IDisposable to do so and that's pretty rare unless you're scoped. We can let the GC handle those objects.
|
This halves allocations for DI |
There was a problem hiding this comment.
You can make this code a little clearer by getting rid of the while and changing the TryAdd to GetOrAdd(key, value). If your add fails, it's because someone was contending with you to also add a value and so you'll get the value they added.
There was a problem hiding this comment.
Feels like this is easier to read.
There was a problem hiding this comment.
Follow your ❤️ - but this gives me the impression that it will run an arbitrary number of times, but it will only ever run 1.5 times at most.
|
overall looks good - main point here seems to be assuming that the service provider is low-contention and we can do simpler/lighter things because of that. |
There was a problem hiding this comment.
If it's internal code, consider removing these exceptions and replacing it with Debug.Assert instead.
|
788k -> 384k (-51%) If you didn't want to go full pooling, but were ok with partial pooling, you could pool the dictionary and list and clear them up in the disposable; which would trim it to 144k but limit the scope for the pooling. 788k -> 144k (-82%) Though likely with that you'd need to return to an object for locking and add bool for dispose check |
|
updated. Going to try a few more things:
|
- Only use it for transient objects - Use _resolvedServices as a lockObject to avoid allocating a lockObj
|
@benaadams New changes for ya. |
|
Looks |
to me. Will wait for
|
Improved: And down overall for the module/dll: 321k vs 401k on original PR so haven't put it back on somewhere else. |
|
Merged |
5 participants
Measurements on the way...
/cc @rynowak @halter73 @pranavkm @lodejard @benaadams