Source generator should share property-level custom converters

[steveharter]

During warm-up of metadata that have the [JsonConverter] attribute applied to properties, converter instances are created for each property and not shared. This primarily increases memory consumption, but also has a small impact on warm-up CPU performance. Note that the current semantics of source-gen are the same as the run-time serializer, so addressing this issue would improve on the run-time serializer.

This does not apply to converters applied to a Type, such as MyDataTypeConverter shown in the sample below, since the Type metadata (which includes the custom converter) is shared across the context.

The two cases where this applies:

1. For a custom factory converter, such as the built-in JsonStringEnumConverter, the factory instance is created for every property. Note that to scope this issue, we don't need to pursue caching resulting instances from factory.CreateConverter() since that sharing logic would need to occur at run-time (e.g. with a dictionary), and not determined at source-gen time.
2. For a custom non-factory converter specified on a property, such as MyEmptyStringConverter shown in the sample below, an instance is created for every property.

For both cases, the run-time serializer does not cache\share these because it is possible to have a custom attribute type that derives from JsonConverterAttribute and overrides CreateConverter(Type) and thus the custom attribute may create a semantically different converter given the Type and any properties specified on the custom attribute. However, since source-gen can spend extra time probing it is possible source-gen can improve on the run-time serializer by checking if the attribute is the standard JsonConverterAttribute or if it is a derived attribute type. If it is the standard JsonConverterAttribute, then converter instances could be shared, and can be shared without having to use a dictionary at run-time to look up the converter.

Sample types:

using System.Text.Json;
using System.Text.Json.Serialization;

string json = JsonSerializer.Serialize(new MyPoco(), MySerializerContext.Default.MyPoco);
Console.WriteLine(json); // {"MyEnum":"One","MyString1":"NULL",null,"MyString2":"NULL",null,"MyDataType1":1,"MyDataType2":2}

[JsonSerializable(typeof(MyPoco))]
public partial class MySerializerContext : JsonSerializerContext
{
}

public class MyPoco
{
    // Factory converter instance is created and would not be shared with other usages:
    [JsonConverter(typeof(JsonStringEnumConverter))]
    public MyEnum MyEnum { get; set; } = MyEnum.One;

    // Converter instance is created for each property:
    [JsonConverter(typeof(MyEmptyStringConverter))]
    public string MyString1 { get; set; } = null;
    [JsonConverter(typeof(MyEmptyStringConverter))]
    public string MyString2 { get; set; } = null;

    // These are cached as expected:
    public MyDataType MyDataType1 { get; set; } = new MyDataType { internalValue = 1 };
    public MyDataType MyDataType2 { get; set; } = new MyDataType { internalValue = 2 };
}

public enum MyEnum
{
    One = 1
}

[JsonConverter(typeof(MyDataTypeConverter))]
public struct MyDataType
{
    public int internalValue;
}

public class MyEmptyStringConverter : JsonConverter<string>
{
    public override bool HandleNull => true;

    public override string Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        reader.Read();

        if (reader.TokenType == JsonTokenType.Null)
        {
            // Change semantics of the default string converter
            return "NULL";
        }
        
        return reader.GetString()!;
    }

    public override void Write(Utf8JsonWriter writer, string value, JsonSerializerOptions options)
    {
        if (value is null)
        {
            // Change semantics of the default string converter
            writer.WriteStringValue("NULL");
        }

        writer.WriteStringValue(value);
    }
}

public class MyDataTypeConverter : JsonConverter<MyDataType>
{
    public override MyDataType Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        reader.Read();
        int value = reader.GetInt32();
        return new MyDataType() { internalValue = value };
    }

    public override void Write(Utf8JsonWriter writer, MyDataType value, JsonSerializerOptions options)
    {
        writer.WriteNumberValue(value.internalValue);
    }
}

with their currently generated code showing the extra converter instances:

public partial class MySerializerContext
{
    ...
private static global::System.Text.Json.Serialization.Metadata.JsonPropertyInfo[] MyPocoPropInit(global::System.Text.Json.Serialization.JsonSerializerContext context)
{
    global::MySerializerContext jsonContext = (global::MySerializerContext)context;
    global::System.Text.Json.JsonSerializerOptions options = context.Options;

    global::System.Text.Json.Serialization.Metadata.JsonPropertyInfo[] properties = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfo[5];

    properties[0] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::MyEnum>(
        options,
...        converter: jsonContext.GetConverterFromFactory<global::MyEnum>(new global::System.Text.Json.Serialization.JsonStringEnumConverter()),
        getter: static (obj) => ((global::MyPoco)obj).MyEnum
...);
    
    properties[1] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::System.String>(
...
        converter: new global::MyEmptyStringConverter(),
...);
    
    properties[2] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::System.String>(
        options,
  ...
      converter: new global::MyEmptyStringConverter(),
...);
    
    properties[3] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::MyDataType>(
...
        converter: null,
...);
    
    properties[4] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::MyDataType>(
...
        converter: null,
...);
    
    return properties;
}

public partial class MySerializerContext
{
    private global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::MyDataType>? _MyDataType;
    public global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::MyDataType> MyDataType
    {
        get
        {
...
                 global::System.Text.Json.Serialization.JsonConverter converter = new global::MyDataTypeConverter();
...
                 _MyDataType = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreateValueInfo<global::MyDataType> (Options, converter); 
       }

        return _MyDataType;
    }
}
    ...
    private global::System.Text.Json.Serialization.JsonConverter<T> GetConverterFromFactory<T>(global::System.Text.Json.Serialization.JsonConverterFactory factory)
    {
        return (global::System.Text.Json.Serialization.JsonConverter<T>) GetConverterFromFactory(typeof(T), factory);
    }
    
    private global::System.Text.Json.Serialization.JsonConverter GetConverterFromFactory(global::System.Type type, global::System.Text.Json.Serialization.JsonConverterFactory factory)
    {
        global::System.Text.Json.Serialization.JsonConverter? converter = factory.CreateConverter(type, Options);
        if (converter == null || converter is global::System.Text.Json.Serialization.JsonConverterFactory)
        {
            throw new global::System.InvalidOperationException($"The converter '{factory.GetType()}' cannot return null or a JsonConverterFactory instance.");
        }
         
        return converter;
    }
}

[ghost]

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

Issue Details

---

During warm-up of metadata that have the [JsonConverter] attribute applied to properties, converter instances are created for each property and not shared. This primarily increases memory consumption, but also has a small impact on warm-up CPU performance. Note that the current semantics of source-gen are the same as the run-time serializer, so addressing this issue would improve on the run-time serializer.

This does not apply to converters applied to a Type, such as MyDataTypeConverter shown in the sample below, since the Type metadata (which includes the custom converter) is shared across the context.

The two cases where this applies:

1. For a custom factory converter, such as the built-in JsonStringEnumConverter, the factory instance is created for every property. Note that to scope this issue, we don't need to pursue caching resulting instances from factory.CreateConverter() since that sharing logic would need to occur at run-time (e.g. with a dictionary), and not determined at source-gen time.
2. For a custom non-factory converter specified on a property, such as MyEmptyStringConverter shown in the sample below, an instance is created for every property.

For both cases, the run-time serializer does not cache\share these because it is possible to have a custom attribute type that derives from JsonConverterAttribute and overrides CreateConverter(Type) and thus the custom attribute may create a semantically different converter given the Type and any properties specified on the custom attribute. However, since source-gen can spend extra time probing it is possible source-gen can improve on the run-time serializer by checking if the attribute is the standard JsonConverterAttribute or if it is a derived attribute type. If it is the standard JsonConverterAttribute, then converter instances could be shared, and can be shared without having to use a dictionary at run-time to look up the converter.

Sample types:

using System.Text.Json;
using System.Text.Json.Serialization;

string json = JsonSerializer.Serialize(new MyPoco(), MySerializerContext.Default.MyPoco);
Console.WriteLine(json); // {"MyEnum":"One","MyString1":"NULL",null,"MyString2":"NULL",null,"MyDataType1":1,"MyDataType2":2}

[JsonSerializable(typeof(MyPoco))]
public partial class MySerializerContext : JsonSerializerContext
{
}

public class MyPoco
{
    // Factory converter instance is created and would not be shared with other usages:
    [JsonConverter(typeof(JsonStringEnumConverter))]
    public MyEnum MyEnum { get; set; } = MyEnum.One;

    // Converter instance is created for each property:
    [JsonConverter(typeof(MyEmptyStringConverter))]
    public string MyString1 { get; set; } = null;
    [JsonConverter(typeof(MyEmptyStringConverter))]
    public string MyString2 { get; set; } = null;

    // These are cached as expected:
    public MyDataType MyDataType1 { get; set; } = new MyDataType { internalValue = 1 };
    public MyDataType MyDataType2 { get; set; } = new MyDataType { internalValue = 2 };
}

public enum MyEnum
{
    One = 1
}

[JsonConverter(typeof(MyDataTypeConverter))]
public struct MyDataType
{
    public int internalValue;
}

public class MyEmptyStringConverter : JsonConverter<string>
{
    public override bool HandleNull => true;

    public override string Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        reader.Read();

        if (reader.TokenType == JsonTokenType.Null)
        {
            // Change semantics of the default string converter
            return "NULL";
        }
        
        return reader.GetString()!;
    }

    public override void Write(Utf8JsonWriter writer, string value, JsonSerializerOptions options)
    {
        if (value is null)
        {
            // Change semantics of the default string converter
            writer.WriteStringValue("NULL");
        }

        writer.WriteStringValue(value);
    }
}

public class MyDataTypeConverter : JsonConverter<MyDataType>
{
    public override MyDataType Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        reader.Read();
        int value = reader.GetInt32();
        return new MyDataType() { internalValue = value };
    }

    public override void Write(Utf8JsonWriter writer, MyDataType value, JsonSerializerOptions options)
    {
        writer.WriteNumberValue(value.internalValue);
    }
}

with their currently generated code showing the extra converter instances:

public partial class MySerializerContext
{
    ...
private static global::System.Text.Json.Serialization.Metadata.JsonPropertyInfo[] MyPocoPropInit(global::System.Text.Json.Serialization.JsonSerializerContext context)
{
    global::MySerializerContext jsonContext = (global::MySerializerContext)context;
    global::System.Text.Json.JsonSerializerOptions options = context.Options;

    global::System.Text.Json.Serialization.Metadata.JsonPropertyInfo[] properties = new global::System.Text.Json.Serialization.Metadata.JsonPropertyInfo[5];

    properties[0] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::MyEnum>(
        options,
...        converter: jsonContext.GetConverterFromFactory<global::MyEnum>(new global::System.Text.Json.Serialization.JsonStringEnumConverter()),
        getter: static (obj) => ((global::MyPoco)obj).MyEnum
...);
    
    properties[1] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::System.String>(
...
        converter: new global::MyEmptyStringConverter(),
...);
    
    properties[2] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::System.String>(
        options,
  ...
      converter: new global::MyEmptyStringConverter(),
...);
    
    properties[3] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::MyDataType>(
...
        converter: null,
...);
    
    properties[4] = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreatePropertyInfo<global::MyDataType>(
...
        converter: null,
...);
    
    return properties;
}

public partial class MySerializerContext
{
    private global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::MyDataType>? _MyDataType;
    public global::System.Text.Json.Serialization.Metadata.JsonTypeInfo<global::MyDataType> MyDataType
    {
        get
        {
...
                 global::System.Text.Json.Serialization.JsonConverter converter = new global::MyDataTypeConverter();
...
                 _MyDataType = global::System.Text.Json.Serialization.Metadata.JsonMetadataServices.CreateValueInfo<global::MyDataType> (Options, converter); 
       }

        return _MyDataType;
    }
}
    ...
    private global::System.Text.Json.Serialization.JsonConverter<T> GetConverterFromFactory<T>(global::System.Text.Json.Serialization.JsonConverterFactory factory)
    {
        return (global::System.Text.Json.Serialization.JsonConverter<T>) GetConverterFromFactory(typeof(T), factory);
    }
    
    private global::System.Text.Json.Serialization.JsonConverter GetConverterFromFactory(global::System.Type type, global::System.Text.Json.Serialization.JsonConverterFactory factory)
    {
        global::System.Text.Json.Serialization.JsonConverter? converter = factory.CreateConverter(type, Options);
        if (converter == null || converter is global::System.Text.Json.Serialization.JsonConverterFactory)
        {
            throw new global::System.InvalidOperationException($"The converter '{factory.GetType()}' cannot return null or a JsonConverterFactory instance.");
        }
         
        return converter;
    }
}

Author:	steveharter
Assignees:	-
Labels:	area-System.Text.Json, tenet-performance
Milestone:	7.0.0

[eiriktsarpalis]

I don't believe such an optimization is worth pursuing -- the added allocations only impact metadata initialization, it can only be used with non-factory declarations, it makes the generated code more complicated to read and maintain, it differs from the semantics of the reflection serializer and it could break users that depend on side-effects of their converter constructors being called individually for each property.