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

Using JSON Source Gen still roots all JsonConverters #52662

Closed
eerhardt opened this issue May 12, 2021 · 1 comment · Fixed by #52673
Closed

Using JSON Source Gen still roots all JsonConverters #52662

eerhardt opened this issue May 12, 2021 · 1 comment · Fixed by #52673
Assignees
Labels
area-System.Text.Json size-reduction Issues impacting final app size primary for size sensitive workloads
Milestone

Comments

@eerhardt
Copy link
Member

Using System.Text.Json source generation in my application isn't providing for size reductions in the app because all the JsonConverters are still being rooted in the app.

Repro

using System;
using System.Text.Json;
using System.Text.Json.Serialization;
using ConsoleApp13;
using ConsoleApp13.JsonSourceGeneration;

[assembly: JsonSerializable(typeof(WeatherForecast))]

namespace ConsoleApp13
{
    class Program
    {
        static void Main(string[] args)
        {
            string f = @"{""TemperatureC"":0,""Summary"":""hi""}";

            var forecast = JsonSerializer.Deserialize(f, new JsonContext().WeatherForecast);
            Console.WriteLine(forecast);
        }
    }

    public record WeatherForecast
    {
        public DateTime Date { get; set; }

        public int TemperatureC { get; set; }

        public string Summary { get; set; }

        public int TemperatureF => 32 + (int)(TemperatureC / 0.5556);
    }
}

dotnet publish -r win-x64 -c Release -p:PublishTrimmed=true -p:DebuggerSupport=false

View the outputted assemblies, and notice that System.Text.Json.dll is ~289KB, it still has all its JsonConverters in it, and System.Collections.Immutable.dll is in my app, even though I don't use it.

Analysis

From looking at the results, the reason everything is still rooted is because of the following call graph:

image

This is what is rooting ObjectConverter. Then inside ObjectConverter.Read, it uses JsonNodeConverter.Instance:

public override object? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
if (options.UnknownTypeHandling == JsonUnknownTypeHandling.JsonElement)
{
return JsonElement.ParseValue(ref reader);
}
return JsonNodeConverter.Instance.Read(ref reader, typeToConvert, options);

This brings in all of the JsonNode hierarchy, including JsonValue<T>, which overrides ToString() to call JsonSerializer.Serialize without source gen information:

JsonSerializer.Serialize(writer, _value, _value!.GetType(), options);

That this point, since JsonSerializer.Serialize is called without source gen information, it now roots all JsonConverters, and the size of the app grows by a lot.

cc @layomia @steveharter @marek-safar @vitek-karas @CoffeeFlux

@eerhardt eerhardt added area-System.Text.Json size-reduction Issues impacting final app size primary for size sensitive workloads labels May 12, 2021
@ghost
Copy link

ghost commented May 12, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Using System.Text.Json source generation in my application isn't providing for size reductions in the app because all the JsonConverters are still being rooted in the app.

Repro

using System;
using System.Text.Json;
using System.Text.Json.Serialization;
using ConsoleApp13;
using ConsoleApp13.JsonSourceGeneration;

[assembly: JsonSerializable(typeof(WeatherForecast))]

namespace ConsoleApp13
{
    class Program
    {
        static void Main(string[] args)
        {
            string f = @"{""TemperatureC"":0,""Summary"":""hi""}";

            var forecast = JsonSerializer.Deserialize(f, new JsonContext().WeatherForecast);
            Console.WriteLine(forecast);
        }
    }

    public record WeatherForecast
    {
        public DateTime Date { get; set; }

        public int TemperatureC { get; set; }

        public string Summary { get; set; }

        public int TemperatureF => 32 + (int)(TemperatureC / 0.5556);
    }
}

dotnet publish -r win-x64 -c Release -p:PublishTrimmed=true -p:DebuggerSupport=false

View the outputted assemblies, and notice that System.Text.Json.dll is ~289KB, it still has all its JsonConverters in it, and System.Collections.Immutable.dll is in my app, even though I don't use it.

Analysis

From looking at the results, the reason everything is still rooted is because of the following call graph:

image

This is what is rooting ObjectConverter. Then inside ObjectConverter.Read, it uses JsonNodeConverter.Instance:

public override object? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
if (options.UnknownTypeHandling == JsonUnknownTypeHandling.JsonElement)
{
return JsonElement.ParseValue(ref reader);
}
return JsonNodeConverter.Instance.Read(ref reader, typeToConvert, options);

This brings in all of the JsonNode hierarchy, including JsonValue<T>, which overrides ToString() to call JsonSerializer.Serialize without source gen information:

JsonSerializer.Serialize(writer, _value, _value!.GetType(), options);

That this point, since JsonSerializer.Serialize is called without source gen information, it now roots all JsonConverters, and the size of the app grows by a lot.

cc @layomia @steveharter @marek-safar @vitek-karas @CoffeeFlux

Author: eerhardt
Assignees: -
Labels:

area-System.Text.Json, size-reduction

Milestone: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label May 12, 2021
@steveharter steveharter self-assigned this May 12, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 12, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 13, 2021
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label May 13, 2021
@layomia layomia added this to the 6.0.0 milestone May 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json size-reduction Issues impacting final app size primary for size sensitive workloads
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants