Get DataContractSerializer to behave nicely with unloadable AssemblyLoadContext #88791
Conversation
…to interfere with what comes next.
…ss that we no longer need.
| where TValue : class? | ||
| { | ||
| private readonly ConcurrentDictionary<TKey, TValue> _fastDictionary = new(); | ||
| private readonly ConditionalWeakTable<TKey, TValue> _collectibleTable = new(); |
There was a problem hiding this comment.
ConditionalWeakTable is pretty fast itself. Have you ran any benchmarks to see if non-unloadable assemblies show an improvement with ConcurrentDictionary?
| } | ||
|
|
||
| internal sealed class ContextAwareDictionary<TKey, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] TValue> | ||
| where TKey : Type |
There was a problem hiding this comment.
Does TKey have to be generic? You could just use Type.
| { | ||
| if (!_collectibleTable.TryGetValue(t, out ret)) | ||
| { | ||
| ret = f(t); |
There was a problem hiding this comment.
Running the delegate inside the lock is prone to deadlocks. Can you move it outside?
There was a problem hiding this comment.
The delegate isn't necessarily a simple amount of work depending on your OM design. It's something you really want to avoid executing multiple times. For example, if you instantiate a DCS on an incoming request on a REST api for example, having 100 concurrent requests could result in redoing this expensive work 100 times. You could negatively impact your first request time significantly.
Can you provide me information about it being prone to deadlocks? The code run by the delegate isn't going to do anything async so any reentrance will occur on the same thread. I don't believe this specific scenario is able to deadlock, but I'm open to learning about ways it can deadlock that I might not be aware of.
There was a problem hiding this comment.
You know better if you think that creating a DCS will not execute arbitrary code. But note that ConcurrentDictionary.GetOrAdd will not execute the delegate inside a lock. In the most common case of non-unloadable assemblies there is still the possibility that the delegate will run many times.
There is also ConditionalWeakTable.CreateValue that you can use like Concurrent.Dictionary.GetOrAdd.
There was a problem hiding this comment.
I agree, we should place a lock around ConcurrentDictionary.GetOrAdd. We don't need to check a second time if we are always holding a lock when adding as that would guarantee the prior add has completed before calling GetOrAdd and it will act like a Get.
|
|
||
| // Common case for collectible contexts | ||
| if (_collectibleTable.TryGetValue(t, out ret)) | ||
| return ret; |
There was a problem hiding this comment.
Need to hold the lock on lookup too
|
Closing in favor of #90437 |
ghost
locked as resolved and limited conversation to collaborators
The first commit in this draft PR adds tests to verify DCS did not work with unloadable ALC's before, and does after.
The second commit is the net effect of PR 73893. As I was working through this it became clear that that PR would not interfere with this ALC work, and actually helps because it moved one of our collections from using TypeHandle to nint. So I started with that as a base before adding on top. I propose we accept that PR since it's been pretty well reviewed at this point.
The third commit is taking the change above and applying the same technique to DCJS.
Everything after is the new work to fix the ALC scenario. Hopefully it's easy to cherry-pick commits and apply them and fix nits after accepting the PR referenced above.
I ran through some super simple perf testing - just on my dev machine. The simple benchmark used to explore PR 73893 shows that the full PR here is on par with the perf gains for
GetIdthat come from PR 73893. There is some jitter - especially at high concurrency - but the two PR's seem to mostly take turns with who comes out the fastest in that simple benchmark. Both show marked improvement over the baseline .net 6/7/8 numbers.I also did an even super-simpler comparison of the various ways to keep an "array" of either strong or weak references to items that can be indexed with an integer. This is what helped me decide to use the named ValueTuple approach in this PR for
s_dataContractCache/ContextAwareIndex. Obviously using a single array of only strong references was the fastest in all cases, but both the two-array approach and the array-of-pairs approach stood out from the other options. The overhead of each is negligible in 0-concurrent/0-weak-reference scenarios, and keeps relative pace with the baseline as concurrency increases. Increasing the number of weak-references does start to show additional overhead, but it's a price that is only paid for unloadable contexts and we can't avoid it.