Remove System.Text.Json's DynamicDependencies on Collections.Immutable

[eerhardt]

Today, JsonSerializer takes a DynamicDependency on basically all the System.Collection.Immutable CreateRange methods:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableConverterFactoryHelpers.cs

Lines 154 to 159 in 5f15498

	[DynamicDependency(CreateRangeMethodNameForEnumerable, ImmutableArrayTypeName, ImmutableCollectionsAssembly)]
	[DynamicDependency(CreateRangeMethodNameForEnumerable, ImmutableListTypeName, ImmutableCollectionsAssembly)]
	[DynamicDependency(CreateRangeMethodNameForEnumerable, ImmutableStackTypeName, ImmutableCollectionsAssembly)]
	[DynamicDependency(CreateRangeMethodNameForEnumerable, ImmutableQueueTypeName, ImmutableCollectionsAssembly)]
	[DynamicDependency(CreateRangeMethodNameForEnumerable, ImmutableSortedSetTypeName, ImmutableCollectionsAssembly)]
	[DynamicDependency(CreateRangeMethodNameForEnumerable, ImmutableHashSetTypeName, ImmutableCollectionsAssembly)]

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableConverterFactoryHelpers.cs

Lines 186 to 187 in 5f15498

	[DynamicDependency(CreateRangeMethodNameForDictionary, ImmutableDictionaryTypeName, ImmutableCollectionsAssembly)]
	[DynamicDependency(CreateRangeMethodNameForDictionary, ImmutableSortedDictionaryTypeName, ImmutableCollectionsAssembly)]

This causes all of the Immutable Collection types to be rooted in an application, even if the app doesn't use the types. This is a fairly substantial amount of code, especially in cases where every KB counts, like Blazor WASM apps.

With #53205, I found out that a bunch of other collection types don't work in JsonSerializer in a trimmed app. That makes me think that these Immutable collection types really shouldn't be treated specially, especially considering they bring in so much unused code.

I prototyped what it would look like removing these DynamicDependencies here. Using this prototype I was able to measure just how much code these DynamicDependencies are causing:

Before: 2,600,754 bytes
After: 2,572,198 bytes

It decreases the app size almost 28KB .br compressed.

With the advent of the JSON source generator, we can make applications that call JsonSerializer trim-safe by telling developers to use the source generator when they need to serialize/deserialize objects. If they care about trim-safety, they can convert their code to use the source generator, and it will work, even after trimming.

However, the problem with removing these attributes is that existing apps that use Immutable Collections in a JSON graph would be broken once they are trimmed. For normal apps, we will give them a warning in all places they call the serializer. However, for Blazor WASM apps, these warnings are suppressed by default. So apps won't see them.

I can think of the following strategies we could take to resolve this:

1. Just remove the DynamicDependencies, and if developers run into issues we tell them to either use the source generator, or tell them which ImmutableCollection methods to root in their app.
2. Introduce a new feature switch, which conditionally adds these DynamicDependencies. By default in Blazor WASM apps, the switch is set to not root these collections. Developers who run into problems in their app can just flip the feature switch, and these collections will be rooted again.
3. Continue down the current path, and if we fix Using Queue, Queue<T>, Stack, and ConcurrentStack<T> as properties in classes deserialized by System.Text.Json does not work when trimmed #53205, JSON will root even more collections that may not even be used in the application.

In my opinion, using Immutable Collections in a JSON graph isn't a super common case. Paying this price in all apps that use JSON isn't worth the tradeoff.

Thoughts?

@steveharter @layomia @eiriktsarpalis @jozkee @vitek-karas @marek-safar @SteveSandersonMS @pranavkm @danroth27 @stephentoub @jkotas

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar, @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Today, JsonSerializer takes a DynamicDependency on basically all the System.Collection.Immutable CreateRange methods:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableConverterFactoryHelpers.cs

Lines 154 to 159 in 5f15498

	[DynamicDependency(CreateRangeMethodNameForEnumerable, ImmutableArrayTypeName, ImmutableCollectionsAssembly)]
	[DynamicDependency(CreateRangeMethodNameForEnumerable, ImmutableListTypeName, ImmutableCollectionsAssembly)]
	[DynamicDependency(CreateRangeMethodNameForEnumerable, ImmutableStackTypeName, ImmutableCollectionsAssembly)]
	[DynamicDependency(CreateRangeMethodNameForEnumerable, ImmutableQueueTypeName, ImmutableCollectionsAssembly)]
	[DynamicDependency(CreateRangeMethodNameForEnumerable, ImmutableSortedSetTypeName, ImmutableCollectionsAssembly)]
	[DynamicDependency(CreateRangeMethodNameForEnumerable, ImmutableHashSetTypeName, ImmutableCollectionsAssembly)]

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableConverterFactoryHelpers.cs

Lines 186 to 187 in 5f15498

	[DynamicDependency(CreateRangeMethodNameForDictionary, ImmutableDictionaryTypeName, ImmutableCollectionsAssembly)]
	[DynamicDependency(CreateRangeMethodNameForDictionary, ImmutableSortedDictionaryTypeName, ImmutableCollectionsAssembly)]

This causes all of the Immutable Collection types to be rooted in an application, even if the app doesn't use the types. This is a fairly substantial amount of code, especially in cases where every KB counts, like Blazor WASM apps.

With #53205, I found out that a bunch of other collection types don't work in JsonSerializer in a trimmed app. That makes me think that these Immutable collection types really shouldn't be treated specially, especially considering they bring in so much unused code.

I prototyped what it would look like removing these DynamicDependencies here. Using this prototype I was able to measure just how much code these DynamicDependencies are causing:

Before: 2,600,754 bytes
After: 2,572,198 bytes

It decreases the app size almost 28KB .br compressed.

With the advent of the JSON source generator, we can make applications that call JsonSerializer trim-safe by telling developers to use the source generator when they need to serialize/deserialize objects. If they care about trim-safety, they can convert their code to use the source generator, and it will work, even after trimming.

However, the problem with removing these attributes is that existing apps that use Immutable Collections in a JSON graph would be broken once they are trimmed. For normal apps, we will give them a warning in all places they call the serializer. However, for Blazor WASM apps, these warnings are suppressed by default. So apps won't see them.

I can think of the following strategies we could take to resolve this:

1. Just remove the DynamicDependencies, and if developers run into issues we tell them to either use the source generator, or tell them which ImmutableCollection methods to root in their app.
2. Introduce a new feature switch, which conditionally adds these DynamicDependencies. By default in Blazor WASM apps, the switch is set to not root these collections. Developers who run into problems in their app can just flip the feature switch, and these collections will be rooted again.
3. Continue down the current path, and if we fix Using Queue, Queue<T>, Stack, and ConcurrentStack<T> as properties in classes deserialized by System.Text.Json does not work when trimmed #53205, JSON will root even more collections that may not even be used in the application.

In my opinion, using Immutable Collections in a JSON graph isn't a super common case. Paying this price in all apps that use JSON isn't worth the tradeoff.

Thoughts?

@steveharter @layomia @eiriktsarpalis @jozkee @vitek-karas @marek-safar @SteveSandersonMS @pranavkm @danroth27 @stephentoub @jkotas

Author:	eerhardt
Assignees:	-
Labels:	area-System.Text.Json, size-reduction
Milestone:	-

[jkotas]

In my opinion, using Immutable Collections in a JSON graph isn't a super common case.

+1

My vote would be for Option 1 Just remove the DynamicDependencies.

[stephentoub]

+1 to Jan's +1.

[marek-safar]

In my opinion, using Immutable Collections in a JSON graph isn't a super common case.

I agree and if you also consider other sensitive workloads that would benefit from this and have lower expectations about pre-net6 compatibility it makes even more sense.