System.Text.Json should support unloadable assemblies correctly

[eiriktsarpalis]

I think this should make the cache work reasonably well with collectible assemblies, but it's hard to tell for sure. Could you please add a test which:

• Creates a new unloadable ALC and loads some assembly with test types into it
• Serializes/deserializes objects of the types from the assembly above
• Unloads the ALC
• Validates that the ALC was actually unloaded

Good guide what to do is here: https://docs.microsoft.com/en-us/dotnet/standard/assembly/unloadability

In short what we want to avoid:
Global GC root which holds onto anything from unloadable ALCs as that will prevent the ALC from unloading. We already have quite a few caches in the FX which do this, so let's not add another one.

Originally posted by @vitek-karas in #64646 (comment)

[ghost]

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

---

I think this should make the cache work reasonably well with collectible assemblies, but it's hard to tell for sure. Could you please add a test which:

• Creates a new unloadable ALC and loads some assembly with test types into it
• Serializes/deserializes objects of the types from the assembly above
• Unloads the ALC
• Validates that the ALC was actually unloaded

Good guide what to do is here: https://docs.microsoft.com/en-us/dotnet/standard/assembly/unloadability

In short what we want to avoid:
Global GC root which holds onto anything from unloadable ALCs as that will prevent the ALC from unloading. We already have quite a few caches in the FX which do this, so let's not add another one.

Originally posted by @vitek-karas in #64646 (comment)

Author:	eiriktsarpalis
Assignees:	-
Labels:	area-AssemblyLoader-coreclr, untriaged
Milestone:	-

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

---

I think this should make the cache work reasonably well with collectible assemblies, but it's hard to tell for sure. Could you please add a test which:

• Creates a new unloadable ALC and loads some assembly with test types into it
• Serializes/deserializes objects of the types from the assembly above
• Unloads the ALC
• Validates that the ALC was actually unloaded

Good guide what to do is here: https://docs.microsoft.com/en-us/dotnet/standard/assembly/unloadability

In short what we want to avoid:
Global GC root which holds onto anything from unloadable ALCs as that will prevent the ALC from unloading. We already have quite a few caches in the FX which do this, so let's not add another one.

Originally posted by @vitek-karas in #64646 (comment)

Author:	eiriktsarpalis
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[eiriktsarpalis]

From #64646 (comment)

In short what we want to avoid:
Global GC root which holds onto anything from unloadable ALCs as that will prevent the ALC from unloading. We already have quite a few caches in the FX which do this, so let's not add another one.

System.Text.Json already suffers from that issue since we're rooting the default JsonSerializerOptions instance (and any caches it may create):

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs

Line 35 in cff924f

public static JsonSerializerOptions Default { get; } = new JsonSerializerOptions { _haveTypesBeenCreated = true };

We need to work out an eviction policy that takes into account the rooted instance as well, however that's not in scope for the current PR, which only addresses the performance issue.

[eiriktsarpalis]

I'm not sure what the best solution would be here. @stephentoub added JsonSerializerOptionsUpdateHandler to support hot reload. Perhaps we should expose something like this for users to call into when unloading their ALC?

[teo-tsirpanis]

Perhaps using ConditionalWeakTable to store the types will help. I will prepare a PR at a later time.

[stephentoub]

Perhaps using ConditionalWeakTable to store the types will help. I will prepare a PR at a later time.

CWT could easily result in types getting dropped from the cache way earlier than they should, just because the GC happened to run, in turn resulting in huge increases in costs for JsonSerializer.

It's possible it could play a role, but any such changes will require very careful measurement.

[teo-tsirpanis]

Can a Type instance get collected before it gets unloaded? I didn't know that. And I see the CWT was considered but turned down in

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs

Line 111 in a25aa66

/// Defines a cache of CachingContexts; instead of using a ConditionalWeakTable which can be slow to traverse

Another thing I thought is to add an Unload event handler on the unloadable ALC of a type whose serialization info is going to get cached, but it needs some care to add only one handler per ALC.

[jkotas]

Can a Type instance get collected before it gets unloaded? I didn't know that.

It cannot. Type instances get collected only once the types gets unloaded.

[stephentoub]

But a reference to a Type will also prevent unloading, even if a weak reference, yes? If that's the case, which I thought it was, then the key to the CWT couldn't be a Type and still have that help with unloadability, at which point something else needs to be the key, and we're back to things getting dropped from the cache more aggressively/randomly than desired. if I'm wrong about a weak reference preventing unloadability, then ignore my comment.

[jkotas]

Type used as the key in CWT won't prevent unloading. You can think about CWT as adding an expando field to the item that is used as the key. CWT does not extend the lifetime of the key, but it keeps the value reachable for as long as the key is reachable.

XmlSerializer has similar global singleton caches and it uses CWT to make them work with unloadable types: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.Xml/src/System/Xml/Serialization/ContextAwareTables.cs#L15.

[jkotas]

BTW: The original System.Text.Json design was trying to avoid these problematic global caches: #28325 . It is unfortunate that we end up reintroducing them.

[stephentoub]

Type used as the key in CWT won't prevent unloading.

Is this specific to CWT or does it apply to weak references in general?

[stephentoub]

The original System.Text.Json design was trying to avoid these problematic global caches: #28325

yes and no. It was keeping the cache in the options instance and saying the user was responsuble for caching, but in the same breath also saying that passing options was optional, which then means the system needs to cache the default for good perf.

[teo-tsirpanis]

Is this specific to CWT or does it apply to weak references in general?

CWT uses dependent handles which are different from weak references. A weak reference on a type/object will not prevent its unloading/collection.

[vitek-karas]

Type used as the key in CWT won't prevent unloading.

Is this specific to CWT or does it apply to weak references in general?

No - unloading is driven by GC - if the Type object (treated as managed object) is collectible, it will be able to unload the assembly/ALC it belongs to. Basically - don't create managed memory leaks is the same thing as allow unloadability. (With the caveat that Type objects normally never go away, since they're tied by the runtime, unless the parent ALC is unloadable).

[stephentoub]

CWT uses dependent handles

Yes, I know that.
https://github.com/dotnet/coreclr/pulls?q=is%3Apr+conditionalweaktable+is%3Aclosed+author%3Astephentoub

But my mental model had long been that the key of a dependent handle is a weak reference, and the dependent nature was from the value to the key. That's not the case?

[teo-tsirpanis]

It is a weak reference, but the types are strongly held elsewhere until and if they are unloaded. When a type needs to be unloaded, it won't be hindered by being in a CWT's key.

[stephentoub]

but the types are strongly held elsewhere until and if they are unloaded

Hence my question about whether the described behavior was specific to CWT or for weak references in general, which Vitek answered. I'm not sure why "CWT uses dependent handles which are different from weak references" is relevant then.

[teo-tsirpanis]

Looks like I had got confused but your questions are answered either way. 😅

If you decide that a CWT is the best way to solve this, I can prepare a PR.

[krwq]

It's unlikely we will have time to address this in 7.0, moving to 8.0

[FlurinBruehwiler]

Hey, I am a beginner and this conversation goes a little bit over my head, but I think I'm running in to this exact issue. I want to unload an assembly, but it fails because I used System.Text.Json to serialize a type from the assembly. So my question is, is there currently a way to clear the cache (or disable it), or do I have to switch to a different Json Serializer?

Update: Got it working now by clearing the caches through reflection.

var assembly = typeof(JsonSerializerOptions).Assembly;
var updateHandlerType = assembly.GetType("System.Text.Json.JsonSerializerOptionsUpdateHandler");
var clearCacheMethod = updateHandlerType?.GetMethod("ClearCache", BindingFlags.Static | BindingFlags.Public);
clearCacheMethod?.Invoke(null, new object?[] { null }); 

[simonferquel]

A note about this, as part of working on CoreCLR support at Unity we encountered this issue. For now we are going to call JsonSerializerOptionsUpdateHandler.ClearCache by reflection as a workaround whenever we unload user code.

[eiriktsarpalis]

Contrary to what I stated in the original post of the issue, the problem with unloading assemblies doesn't lie with the reusable caches implementation (it points to them using weak references) but the fact that we keep default singleton JsonSerializerOptions instances. These can all be cleared via the JsonSerializerOptionsUpdateHandler.ClearCache method which we added to support hot reload, but it seems we could make it public so that other consumers don't need to resort to reflection. cc @stephentoub

[stephentoub]

That effectively promotes what's intended to be an implementation detail to instead be something in the public API. I'd rather explore alternative options, like using a CWT, or if that has measurably negative performance implications, looking at using a CWT only for types in unloadable assemblies.

[eiriktsarpalis]

That effectively promotes what's intended to be an implementation detail to instead be something in the public API.

It's pretty unambiguous in what it does and we have important customers taking a de facto dependency on the current private implementation. We probably couldn't change it much without introducing substantial disruption.

like using a CWT, or if that has measurably negative performance implications, looking at using a CWT only for types in unloadable assemblies.

We could try to measure this, but my concern is that this would still complicate lookup logic (checking if the assembly of the type is unloadable, looking up two separate caches). From my perspective the existing (private) approach is the simplest approach that shouldn't compromise lookup performance.

[stephentoub]

we have important customers taking a de facto dependency on the current private implementation. We probably couldn't change it much without introducing substantial disruption.

We can't be in a situation where we're prevented from changing private APIs because someone is using them via private reflection. Someone doing so is on their own.

[vitek-karas]

Aside from this being a bit weird from a caller's point of view ("Why do I care it uses caches inside... why should I?"), I think this would hurt our unloadability story. It's already a bit challenging because it's cooperative and it's easy to break things by holding onto references too long. The debugging story for this is also not the best (try to find GC roots for things in a given ALC, which currently requires SOS debugging). And even if I did go through all of that and found out that the GC root is inside System.Text.Json, how would I learn that I need to call this API?

Unloadability should work out of the box - if my code doesn't hold onto anything in the ALC, I should be able to unload it. Note that this is not the only case in framework where we have global caches which hold onto types, if we used the same solution in the other places as well, I might need to call several such "Clear" methods every time I want to unload something. I just find that a really weird design choice.

[eiriktsarpalis]

Here's a benchmark comparing lookup performance between CD and CWT for Type keys:

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Collections.Concurrent;
using System.Runtime.CompilerServices;

BenchmarkRunner.Run<Benchmark>();

public class Benchmark
{
    [Params(1, 10, 100, 1000, 2000)]
    public int Count;

    public class TypeToHit;
    public class  TypeToMiss;

    private ConcurrentDictionary<Type, string> _concurrentDict = new();
    private ConditionalWeakTable<Type, string> _conditionalWeakTable = new();

    [GlobalSetup]
    public void Init()
    {
        var types = typeof(int).Assembly.GetTypes()
            .Take(Count - 1)
            .Append(typeof(TypeToHit));

        foreach (Type t in types)
        {
            _concurrentDict[t] = t.Name;
            _conditionalWeakTable.Add(t, t.Name);
        }
    }

    [Benchmark]
    public string? ConcurrentDictionary_Hit() 
        => _concurrentDict.TryGetValue(typeof(TypeToHit), out string? value) ? value : null;

    [Benchmark]
    public string? ConcurrentDictionary_Miss() 
        => _concurrentDict.TryGetValue(typeof(TypeToMiss), out string? value) ? value : null;

    [Benchmark]
    public string? ConditionalWeakTable_Hit() 
        => _conditionalWeakTable.TryGetValue(typeof(TypeToHit), out string? value) ? value : null;

    [Benchmark]
    public string? ConditionalWeakTable_Miss() 
        => _conditionalWeakTable.TryGetValue(typeof(TypeToMiss), out string? value) ? value : null;
}

Results

Method	Count	Mean	Error	StdDev
ConcurrentDictionary_Hit	1	4.457 ns	0.0868 ns	0.0812 ns
ConcurrentDictionary_Miss	1	2.089 ns	0.0742 ns	0.0694 ns
ConditionalWeakTable_Hit	1	6.582 ns	0.0474 ns	0.0370 ns
ConditionalWeakTable_Miss	1	2.330 ns	0.0684 ns	0.0606 ns
ConcurrentDictionary_Hit	10	4.069 ns	0.0591 ns	0.0524 ns
ConcurrentDictionary_Miss	10	1.884 ns	0.0711 ns	0.0665 ns
ConditionalWeakTable_Hit	10	7.105 ns	0.1192 ns	0.0931 ns
ConditionalWeakTable_Miss	10	2.121 ns	0.0416 ns	0.0389 ns
ConcurrentDictionary_Hit	100	4.188 ns	0.1218 ns	0.1139 ns
ConcurrentDictionary_Miss	100	1.964 ns	0.0771 ns	0.0721 ns
ConditionalWeakTable_Hit	100	6.683 ns	0.1510 ns	0.1413 ns
ConditionalWeakTable_Miss	100	2.085 ns	0.0577 ns	0.0540 ns
ConcurrentDictionary_Hit	1000	4.079 ns	0.1199 ns	0.1231 ns
ConcurrentDictionary_Miss	1000	1.882 ns	0.0563 ns	0.0527 ns
ConditionalWeakTable_Hit	1000	6.738 ns	0.1187 ns	0.1110 ns
ConditionalWeakTable_Miss	1000	2.087 ns	0.0582 ns	0.0544 ns
ConcurrentDictionary_Hit	2000	3.996 ns	0.0831 ns	0.0777 ns
ConcurrentDictionary_Miss	2000	1.834 ns	0.0536 ns	0.0501 ns
ConditionalWeakTable_Hit	2000	6.664 ns	0.1163 ns	0.1031 ns
ConditionalWeakTable_Miss	2000	2.116 ns	0.0585 ns	0.0547 ns

Roughly speaking this is showing a 2x slowdown, but I'm not sure how substantially that would register in the context of a full-blown serialization operation. It probably would regress performance in some of our microbenchmarks measuring serialization for small POCOs.

[eiriktsarpalis]

@vitek-karas are there any circumstances beyond assembly unload events that could result in Type instances getting collected? We'd want to avoid the possibility of needing to recompute caches for the same types.

[vitek-karas]

I actually don't know if types built dynamically via TypeBuilder are collectible, if so, those would be the other case.

[jkotas]

types built dynamically via TypeBuilder are collectible

Unloadability is at assembly granularity. The types are collectible if their assembly builder was created using AssemblyBuilderAccess.RunAndCollect.

assembly unload events

I do not think we have public assembly unload event API.

[Delsin-Yu]

Is there any chance we can get this in .Net 9?

[eiriktsarpalis]

Very unlikely at this point.

[Delsin-Yu]

I want to include that the Godot.Net developers are also affected by this issue. It is not a rare case to do Json Serialization/Deserialization in Unlodable Assemblies (for editor tooling).

Now that .Net 9 is out, it would be great if we can hear the team's perspective on this feature and its position on the work list. It is really frustrating to see it postponed each time and eventually put on the future (if not never) list.

Does adding the unloadability to System.Text.Json cause intolerable performance degradation, or is the unloadability for the entire BCL a systematic task that needs to be addressed and should not be patched only for System.Text.Json?

Alternatively, can we reload the CLR to ignore the not unloadable ALCs (like reloading the entire BCL)? I apologize for my lack of knowledge of the low-level runtime design.

[eiriktsarpalis]

I would be inclined to just promote the existing method to a public API. The solution of using a CWT instead of CD is bound to introduce performance regressions -- we already know that CD lookups account for a nontrivial part of the serialization cost so regressing that by 2x per the benchmarks above is not a good look.

[simonferquel]

Another approach would be to subscribe to ALC unload requests: when an entry in the cache is added, look if its owning ALC is already known. If it is not and it is unloadable, subscribe to the unloaded event to automatically clear cache.
This way, lookup complexity is the same, all additional checks are done only once when an entry is added.

[tbdty]

Another approach would be to subscribe to ALC unload requests: when an entry in the cache is added, look if its owning ALC is already known. If it is not and it is unloadable, subscribe to the unloaded event to automatically clear cache. This way, lookup complexity is the same, all additional checks are done only once when an entry is added.

That will not work. There is no Unloaded event, there is an Unloading event. The latter occurs when an unload is requested, not when the actual unload is performed - and an unload will never be performed because you keep holding references to types from the ALC.

[simonferquel]

Unloading is what we want in that case: the ALC is not unloaded because System.Text.Json caches hold onto it: cache needs to be cleared when Unloading is raised so that the ALC can ultimately get unloaded once GC has collected all objects created from it.

[tbdty]

I would be inclined to just promote the existing method to a public API. The solution of using a CWT instead of CD is bound to introduce performance regressions -- we already know that CD lookups account for a nontrivial part of the serialization cost so regressing that by 2x per the benchmarks above is not a good look.

That is a very ugly workaround to the problem at best. You still have to actively be aware of System.Text.Json caching for unloading to work properly.

What about the solution suggested in #65323 (comment)? In this solution, you would maintain two caches: The current default one, and one for collectible types that uses a CWT. Things would "just work" and you would only pay the performance penalty if you actually use collectible assemblies.

[jkotas]

The solution of using a CWT instead of CD is bound to introduce performance regressions

Can this be pay-for-play? Use CWT for unloadable types only?

[simonferquel]

@tbdty please reconsider the option of subscribing to the unloading event, this is actually the event needed (we are trying to prevent ALC to be kept alive, so we need to clear when Unloading is requested, not when it actually completes - which wont happen unless we clear the cache)

[tbdty]

Unloading is what we want in that case: the ALC is not unloaded because System.Text.Json caches hold onto it: cache needs to be cleared when Unloading is raised so that the ALC can ultimately get unloaded once GC has collected all objects created from it.

That is not how ALC works, there is no trigger to unload. The documentation even explicitly states that Unload is just a signal for the runtime to allow unloading as soon as it becomes possible. In my main use case, I actually create a collectible ALC, load an assembly from it, return that assembly and immediately call Unload() in the finally block. The references to the actual assembly may be dropped days or weeks later and the assembly is then collected.

Even if you don't do it like this, your suggestion immediately breaks if someone calls Unload and then directly calls JsonSerializer.Deserialize<TypeFromThatAssembly>, because the cache is cleared when Unload is called, but a new cache entry is added afterwards. Getting this right will be a nightmare and extremely hard to debug.

The entire point of collectible ALC is that the GC is capable of detecting when unloading can happen. By keeping these caches, we are preventing that mechanism from working.

[Delsin-Yu]

It would be great to have something that can operate at the ALC level, like [ALCStatic] attribute on a field, but that's certainly a runtime change.

[jkotas]

It would be great to have something that can operate at the ALC level, like [ALCStatic] attribute on a field, but that's certainly a runtime change.

It would have similar overhead as conditional weak table. Also, I do not see how this can work here. The field that's keeping the ALC alive is in concurrent dictionary.

[Delsin-Yu]

The field that's keeping the ALC alive is in concurrent dictionary.

Ah, you are correct; the calling assembly is different so it won't work anyway.