Use instance methods for SerializeHandler in source gen

[MichalStrehovsky]

S.T.Json source generator generates static void *SerializeHandler(...) methods that perform serialization. These are invoked through a the SerializeHandler delegate on JsonObjectInfoValues<T>.

Invoking delegate to a static method is slower than invoking a delegate to an instance methods in .NET. Invocation of delegates to instance methods have comparable perf characteristics to a virtual call. Invocation of delegate to static methods goes through a shuffle thunk that deals with calling convention incompatibilities at the callsite (that assumes instance method) vs target (static method).

I ran the source generator, captured the outputs in *.cs files and changed this method:

            private static void JsonMessage2SerializeHandler(global::System.Text.Json.Utf8JsonWriter writer, global::JsonMessage2 value)
            {

                writer.WriteStartObject();
                writer.WriteString(PropName_message, value.message);

                writer.WriteEndObject();
            }

To not be static but be instance instead. Here's the difference when serializing a simple struct with a string field (the scenario used in TechEmpower benchmarks):

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19043.1348 (21H1/May2021Update)
Intel Core i7-4870HQ CPU 2.50GHz (Haswell), 1 CPU, 8 logical and 4 physical cores
.NET SDK=6.0.100-rc.2.21505.57
  [Host]   : .NET 6.0.0 (6.0.21.48005), X64 RyuJIT
  .NET 6.0 : .NET 6.0.0 (6.0.21.48005), X64 RyuJIT

Job=.NET 6.0  Runtime=.NET 6.0  

Method	Mean	Error	StdDev
StaticMethod	102.89 ns	0.214 ns	0.189 ns
InstanceMethod	99.93 ns	0.337 ns	0.299 ns

I think the change needs to be done here, but I don't know how to test the source generators to ensure it's generating what I think it's generating, so I'm just filing an issue.

runtime/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs

Lines 1046 to 1050 in 784687f

	private static void {serializeMethodName}({Utf8JsonWriterTypeRef} {WriterVarName}, {valueTypeRef} {ValueVarName})
	{{
	{GetEarlyNullCheckSource(emitNullCheck)}
	{serializeMethodBody}
	}}";

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

---

S.T.Json source generator generates static void *SerializeHandler(...) methods that perform serialization. These are invoked through a the SerializeHandler delegate on JsonObjectInfoValues<T>.

Invoking delegate to a static method is slower than invoking a delegate to an instance methods in .NET. Invocation of delegates to instance methods have comparable perf characteristics to a virtual call. Invocation of delegate to static methods goes through a shuffle thunk that deals with calling convention incompatibilities at the callsite (that assumes instance method) vs target (static method).

I ran the source generator, captured the outputs in *.cs files and changed this method:

            private static void JsonMessage2SerializeHandler(global::System.Text.Json.Utf8JsonWriter writer, global::JsonMessage2 value)
            {

                writer.WriteStartObject();
                writer.WriteString(PropName_message, value.message);

                writer.WriteEndObject();
            }

To not be static but be instance instead. Here's the difference when serializing a simple struct with a string field (the scenario used in TechEmpower benchmarks):

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19043.1348 (21H1/May2021Update)
Intel Core i7-4870HQ CPU 2.50GHz (Haswell), 1 CPU, 8 logical and 4 physical cores
.NET SDK=6.0.100-rc.2.21505.57
  [Host]   : .NET 6.0.0 (6.0.21.48005), X64 RyuJIT
  .NET 6.0 : .NET 6.0.0 (6.0.21.48005), X64 RyuJIT

Job=.NET 6.0  Runtime=.NET 6.0  

Method	Mean	Error	StdDev
StaticMethod	102.89 ns	0.214 ns	0.189 ns
InstanceMethod	99.93 ns	0.337 ns	0.299 ns

I think the change needs to be done here, but I don't know how to test the source generators to ensure it's generating what I think it's generating, so I'm just filing an issue.

runtime/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs

Lines 1046 to 1050 in 784687f

	private static void {serializeMethodName}({Utf8JsonWriterTypeRef} {WriterVarName}, {valueTypeRef} {ValueVarName})
	{{
	{GetEarlyNullCheckSource(emitNullCheck)}
	{serializeMethodBody}
	}}";

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

[eiriktsarpalis]

It seems like a doable refactoring, I'd initially assumed that the JsonTypeInfo<T> properties were static, but apparently they are not.

Would it be possible to share your full benchmark code? The performance gains don't seem particularly impressive for what appears to be a very small POCO, so I don't know if performance gains provide sufficient motivation for us to go ahead with this refactoring. Note that delegate invocation is only used for the root-level object, the generated fast-path serialization methods generally compose using regular method calls:

        private static void FooSerializeHandler(global::System.Text.Json.Utf8JsonWriter writer, global::Foo? value)
        {
            if (value == null)
            {
                writer.WriteNullValue();
                return;
            }
        
            writer.WriteStartObject();
            writer.WritePropertyName(PropName_Bar);
            BarSerializeHandler(writer, value.Bar!);
        
            writer.WriteEndObject();
        }

cc @layomia

[MichalStrehovsky]

Here's the benchmark: BdnJson.zip. The only difference between JsonMessage1 and JsonMessage2 is that one of them has an instance method as the handler.

The performance gains don't seem particularly impressive for what appears to be a very small POCO

FWIW, the POCO in use is the same one that is in use in TechEmpower benchmarks and we tend to be interested in the perf of those.

[layomia]

For completeness I'd like to first add a new variant of the JsonSerializer benchmarks (dotnet/performance#2153), then validate the change to instance methods against that.

[steveharter]

For additional background on shuffle thunk see #45232 (comment)

I believe for every additional parameter over the count of 1 there will be a slight improvement.