Skip to content

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

Closed

Description

@eerhardt

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

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

[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

Activity

  1. ghost added
    untriagedNew issue has not been triaged by the area owner
    on May 25, 2021
  2. jkotas commented on May 26, 2021

    @jkotas
    Member

    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.

  3. stephentoub commented on May 26, 2021

    @stephentoub
    Member

    +1 to Jan's +1.

  4. marek-safar commented on May 26, 2021

    @marek-safar
    Contributor

    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.

  5. added this to the 6.0.0 milestone on May 26, 2021
  6. removed
    untriagedNew issue has not been triaged by the area owner
    on May 26, 2021
  7. self-assigned this
    on May 26, 2021
  8. added a commit that references this issue on May 26, 2021
    685615e
  9. ghost added
    in-prThere is an active PR which will close this issue when it is merged
    on May 26, 2021
  10. added a commit that references this issue on May 27, 2021
    fe67c3f
  11. ghost removed
    in-prThere is an active PR which will close this issue when it is merged
    on May 27, 2021
  12. ghost locked as resolved and limited conversation to collaborators on Jun 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions