Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support linker removal of unused converters #36782

Closed
steveharter opened this issue May 20, 2020 · 4 comments
Closed

Support linker removal of unused converters #36782

steveharter opened this issue May 20, 2020 · 4 comments
Assignees
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Text.Json size-reduction Issues impacting final app size primary for size sensitive workloads
Milestone

Comments

@steveharter
Copy link
Member

When building a standalone app, unused converters should be linked out. This helps to reduce assembly size, increase startup perf (no unnecessary JITing) and reduced private bytes (less JITing and less objects in memory). These are goals for 5.0 as documented here.

Each converter that uses a value type (e.g. Int64Converter) saves ~10K of assembly size after crossgen and each converter that uses a reference type (e.g. ArrayConverter) saves ~.5K of assembly size after crossgen.

In order to help trim references a new API is necessary:

public class JsonSerializerOptions
{
...
    public static JsonSerializerOptions CreateWithNoConverters();
...
}

This creates an options instance that has no built-in converters. It doesn't "root" any of the converters so those converters can linked out.

Converters are then added manually through the existing Converters.Add() method by the calling code that initializes the options or by applying the [JsonConverter] attribute to properties or perhaps to an Assembly (requires a change to that attribute so it can target an Assembly).

Calling Converters.Add() and\or using [JsonConverter] to root the converter types is intended to be done through a new code-gen feature but could also be leveraged by those who want the smallest possible standalone app size.

This also means the existing converters that are now internal in the System.Text.Json.Serialization.Converters namespace will need to be made public so they can be referenced. Currently there are ~46 converters.

@steveharter steveharter added api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Text.Json labels May 20, 2020
@steveharter steveharter added this to the 5.0 milestone May 20, 2020
@steveharter steveharter self-assigned this May 20, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label May 20, 2020
@steveharter steveharter removed the untriaged New issue has not been triaged by the area owner label May 20, 2020
@jkotas jkotas added the linkable-framework Issues associated with delivering a linker friendly framework label May 20, 2020
@stephentoub
Copy link
Member

This also means the existing converters that are now internal in the System.Text.Json.Serialization.Converters namespace will need to be made public so they can be referenced. Currently there are ~46 converters.

That's a ton of new public surface area that also exposes what is essentially implementation detail.

Is this path not going to become immediate legacy with source generators?

If this route is really important to pursue, could we instead, for example, expose a single method like:

public void AddImplicitConverter<T>()

The implementation would switch on the type, and I believe the linker should be able to trim away anything not related to the specific type used.

Also, 10K seems like a lot. Maybe it's all necessary, but have we looked to see where it's coming from and whether there are ways to reduce it, aside from this proposal?

@eerhardt
Copy link
Member

This work isn't necessary for 5.0, so moving to 6.0.

@eerhardt
Copy link
Member

Do we still need this issue now that we have JSON Source Generator?

@layomia layomia self-assigned this Jun 9, 2021
@layomia
Copy link
Contributor

layomia commented Jun 9, 2021

The JSON source generator helps us achieve this - #45448.

@layomia layomia closed this as completed Jun 9, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Text.Json size-reduction Issues impacting final app size primary for size sensitive workloads
Projects
None yet
Development

No branches or pull requests

7 participants