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

System.Text.Json: .NET 5.0 perf regression serializing Nullable<T> #46788

Merged
merged 2 commits into from
Jan 15, 2021

Conversation

CodeBlanch
Copy link
Contributor

I ran into these perf testing some other stuff.

Nullable Property

		public static void Main()
		{
			for (int i = 0; i < 100000; i++)
			{
				JsonSerializer.Serialize(new TestClass { Timestamp = DateTime.UtcNow });
			}
		}

		public class TestClass
		{
			public DateTime? Timestamp { get; set; }
		}

That code running on .NET 5 allocates a whole bunch of DateTimes:

image

Same code running on .NET 3.1 doesn't allocate any.

I tracked it down to this:

That null check boxes the DateTime. Interestingly it works fine for DateTime but not Nullable<DateTime>. A quirk of the cast operation on Nullable<T> maybe? This PR has a fix (virtual bool IsNull), but there might be a better way to do it.

Benchmarks showing the regression:

Method Job Runtime Toolchain Mean Error StdDev Median Min Max Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
SerializeToString Job-TTOALK .NET 6.0 net6.0 451.9 ns 7.66 ns 6.40 ns 450.7 ns 440.2 ns 463.4 ns 0.94 0.03 0.0457 - - 392 B
SerializeToString Job-WBBMJH .NET Core 3.1 netcoreapp3.1 480.2 ns 14.02 ns 15.00 ns 476.7 ns 464.3 ns 516.8 ns 1.00 0.00 0.0430 - - 368 B
SerializeToUtf8Bytes Job-TTOALK .NET 6.0 net6.0 410.8 ns 8.85 ns 9.84 ns 409.5 ns 393.8 ns 427.5 ns 0.97 0.03 0.0352 - - 296 B
SerializeToUtf8Bytes Job-WBBMJH .NET Core 3.1 netcoreapp3.1 425.0 ns 8.09 ns 7.57 ns 423.2 ns 415.1 ns 440.6 ns 1.00 0.00 0.0322 - - 272 B
SerializeToStream Job-TTOALK .NET 6.0 net6.0 510.3 ns 6.03 ns 5.64 ns 511.2 ns 500.6 ns 517.0 ns 1.02 0.02 0.0208 - - 176 B
SerializeToStream Job-WBBMJH .NET Core 3.1 netcoreapp3.1 500.4 ns 8.61 ns 7.64 ns 501.2 ns 491.3 ns 516.3 ns 1.00 0.00 0.0179 - - 152 B
SerializeObjectProperty Job-TTOALK .NET 6.0 net6.0 675.2 ns 11.48 ns 10.18 ns 674.2 ns 657.8 ns 694.5 ns 1.01 0.02 0.0844 - - 720 B
SerializeObjectProperty Job-WBBMJH .NET Core 3.1 netcoreapp3.1 665.9 ns 8.81 ns 7.81 ns 665.6 ns 654.5 ns 677.2 ns 1.00 0.00 0.0788 - - 664 B

Benchmarks with fix:

Method Mean Error StdDev Median Min Max Gen 0 Gen 1 Gen 2 Allocated
SerializeToString 395.9 ns 7.42 ns 6.94 ns 394.6 ns 383.6 ns 411.4 ns 0.0440 - - 368 B
SerializeToUtf8Bytes 351.0 ns 6.26 ns 5.86 ns 351.2 ns 342.1 ns 363.7 ns 0.0317 - - 272 B
SerializeToStream 462.9 ns 11.29 ns 12.08 ns 459.6 ns 448.0 ns 490.5 ns 0.0179 - - 152 B
SerializeObjectProperty 631.9 ns 7.32 ns 6.85 ns 630.4 ns 619.8 ns 646.9 ns 0.0810 - - 696 B

Nullable Root

		public static void Main()
		{
			for (int i = 0; i < 100000; i++)
			{
				JsonSerializer.Serialize((DateTime?)DateTime.UtcNow);
			}
		}

This code allocates on both .NET 3.1 & .NET 5.

I tracked it down to this:

Same idea here, null check is causing a box op. Easy enough to fix just reversing the clauses, included on this PR.

Benchmarks of existing perf:

Method Job Runtime Toolchain Mean Error StdDev Median Min Max Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
SerializeToString Job-MMTVPI .NET 6.0 net6.0 339.8 ns 18.53 ns 20.59 ns 335.9 ns 317.4 ns 391.3 ns 0.90 0.06 0.0334 - - 280 B
SerializeToString Job-IMRGBN .NET Core 3.1 netcoreapp3.1 375.2 ns 14.27 ns 16.44 ns 373.3 ns 357.3 ns 410.9 ns 1.00 0.00 0.0310 - - 264 B
SerializeToUtf8Bytes Job-MMTVPI .NET 6.0 net6.0 297.5 ns 1.99 ns 1.77 ns 297.6 ns 294.0 ns 300.4 ns 0.88 0.03 0.0297 - - 256 B
SerializeToUtf8Bytes Job-IMRGBN .NET Core 3.1 netcoreapp3.1 340.4 ns 9.70 ns 10.37 ns 338.9 ns 323.8 ns 363.1 ns 1.00 0.00 0.0277 - - 232 B
SerializeToStream Job-MMTVPI .NET 6.0 net6.0 329.8 ns 5.68 ns 5.31 ns 331.4 ns 316.6 ns 334.8 ns 0.83 0.01 0.0200 - - 176 B
SerializeToStream Job-IMRGBN .NET Core 3.1 netcoreapp3.1 400.7 ns 7.06 ns 5.51 ns 400.5 ns 392.7 ns 409.6 ns 1.00 0.00 0.0210 - - 176 B
SerializeObjectProperty Job-MMTVPI .NET 6.0 net6.0 353.3 ns 6.87 ns 6.43 ns 352.8 ns 344.0 ns 365.7 ns 0.88 0.02 0.0295 - - 256 B
SerializeObjectProperty Job-IMRGBN .NET Core 3.1 netcoreapp3.1 402.3 ns 6.81 ns 6.37 ns 401.2 ns 394.2 ns 412.0 ns 1.00 0.00 0.0305 - - 256 B

Benchmarks with tweak:

Method Mean Error StdDev Median Min Max Gen 0 Gen 1 Gen 2 Allocated
SerializeToString 224.6 ns 3.67 ns 3.25 ns 223.8 ns 220.0 ns 230.4 ns 0.0286 - - 240 B
SerializeToUtf8Bytes 199.6 ns 3.94 ns 3.87 ns 198.7 ns 195.1 ns 205.9 ns 0.0244 - - 208 B
SerializeToStream 280.0 ns 5.11 ns 4.26 ns 280.9 ns 273.7 ns 287.2 ns 0.0179 - - 152 B
SerializeObjectProperty 352.8 ns 4.30 ns 4.02 ns 353.2 ns 344.9 ns 359.9 ns 0.0294 - - 256 B

Micro Benchmarks

If this PR merges I will open a PR with these new cases in perf repo:

    [GenericTypeArguments(typeof(DateTime?))]
    [GenericTypeArguments(typeof(ClassWithValueTypes))]
    public class WriteJson<T>

    public class ClassWithValueTypes
    {
        public DateTime Timestamp { get; set; }

        public DateTime? NullableTimestamp { get; set; }
    }

That is what I used for the above.

@@ -70,5 +70,7 @@ internal override void WriteNumberWithCustomHandling(Utf8JsonWriter writer, T? v
_converter.WriteNumberWithCustomHandling(writer, value.Value, handling);
}
}

internal override bool IsNull(T? value) => !value.HasValue;
Copy link
Member

@steveharter steveharter Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having to do this seems a bit strange. I'll do some local prototyping to see why is going on here -- boxing issues with Nullable<T> or otherwise. If there's an issue with Nullable<T> then we should try to fix it in the runtime.

@steveharter
Copy link
Member

It appears the culprit is the in modifier + Nullable<T> + == operator.

When using == boxing occurs and when not using in a copy is made via ldobj probably because Nullable<T> is not adorned with the readonly modifier. I'll look at creating an issue for Nullable once I investigate a bit more.

The fastest approach for Nullable<T> is using the in + the Equals() method. However, since we don't want to call Equals() for reference types (since they could overload perhaps and have do something weird that makes semantics inconsistent) keeping your IsNull() approach but adding the in modifier seems like the best route.

However it would be good to check if a custom value type (without readonly modifier) would be slow here; perhaps a call to CanBeNull() would be better before calling IsNull() to prevent the method call?

Simple repro (click to expand)
    class Program
    {
        const long Iterations = 100_000_000;

        public static void Main()
        {
            var converter = new NullableConverter<int>();

            // warm-up (ignore results here)
            EqualsOperator(converter);

            EqualsOperator(converter);
            EqualsOperator_In(converter);
            IsOperator(converter);
            IsOperator_In(converter);
            EqualsMethod(converter);
            EqualsMethod_In(converter);
            VirtualMethod(converter);
            VirtualMethod_In(converter);
        }

        public static void EqualsOperator(JsonConverter<Nullable<int>> converter)
        {
            int? value = 1;

            var sw = new Stopwatch();
            sw.Start();

            for (long i = 0; i < Iterations; i++)
            {
                converter.EqualsOperator(value);
            }
            sw.Stop();

            Console.WriteLine($"EqualsOperator {sw.ElapsedMilliseconds}");
        }

        public static void EqualsOperator_In(JsonConverter<Nullable<int>> converter)
        {
            int? value = 1;

            var sw = new Stopwatch();
            sw.Start();

            for (long i = 0; i < Iterations; i++)
            {
                converter.EqualsOperator_In(value);
            }
            sw.Stop();

            Console.WriteLine($"EqualsOperator_In {sw.ElapsedMilliseconds}");
        }

        public static void IsOperator(JsonConverter<Nullable<int>> converter)
        {
            int? value = 1;

            var sw = new Stopwatch();
            sw.Start();

            for (long i = 0; i < Iterations; i++)
            {
                converter.IsOperator(value);
            }
            sw.Stop();

            Console.WriteLine($"IsOperator {sw.ElapsedMilliseconds}");
        }

        public static void IsOperator_In(JsonConverter<Nullable<int>> converter)
        {
            int? value = 1;

            var sw = new Stopwatch();
            sw.Start();

            for (long i = 0; i < Iterations; i++)
            {
                converter.IsOperator_In(value);
            }
            sw.Stop();

            Console.WriteLine($"IsOperator_In {sw.ElapsedMilliseconds}");
        }

        public static void EqualsMethod(JsonConverter<Nullable<int>> converter)
        {
            int? value = 1;

            var sw = new Stopwatch();
            sw.Start();

            for (long i = 0; i < Iterations; i++)
            {
                converter.EqualsMethod(value);
            }
            sw.Stop();

            Console.WriteLine($"EqualsMethod {sw.ElapsedMilliseconds}");
        }

        public static void EqualsMethod_In(JsonConverter<Nullable<int>> converter)
        {
            int? value = 1;

            var sw = new Stopwatch();
            sw.Start();

            for (long i = 0; i < Iterations; i++)
            {
                converter.EqualsMethod_In(value);
            }
            sw.Stop();

            Console.WriteLine($"EqualsMethod_In {sw.ElapsedMilliseconds}");
        }

        public static void VirtualMethod(JsonConverter<Nullable<int>> converter)
        {
            int? value = 1;

            var sw = new Stopwatch();
            sw.Start();

            for (long i = 0; i < Iterations; i++)
            {
                converter.VirtualMethod(value);
            }
            sw.Stop();

            Console.WriteLine($"VirtualMethod {sw.ElapsedMilliseconds}");
        }

        public static void VirtualMethod_In(JsonConverter<Nullable<int>> converter)
        {
            int? value = 1;

            var sw = new Stopwatch();
            sw.Start();

            for (long i = 0; i < Iterations; i++)
            {
                converter.VirtualMethod_In(value);
            }
            sw.Stop();

            Console.WriteLine($"VirtualMethod_In {sw.ElapsedMilliseconds}");
        }

        public abstract class JsonConverter<T>
        {
            public void EqualsOperator(T value)
            {
                if (value == null)
                {
                    throw new Exception("shouldn't get here");
                }
            }

            public void EqualsOperator_In(in T value)
            {
                if (value == null)
                {
                    throw new Exception("shouldn't get here");
                }
            }

            public void IsOperator(T value)
            {
                if (value is null)
                {
                    throw new Exception("shouldn't get here");
                }
            }

            public void IsOperator_In(in T value)
            {
                if (value is null)
                {
                    throw new Exception("shouldn't get here");
                }
            }

            public void EqualsMethod(T value)
            {
                if (value.Equals(null))
                {
                    throw new Exception("shouldn't get here");
                }
            }

            public void EqualsMethod_In(in T value)
            {
                if (value.Equals(null))
                {
                    throw new Exception("shouldn't get here");
                }
            }

            public void VirtualMethod(T value)
            {
                if (IsNull(value))
                {
                    throw new Exception("shouldn't get here");
                }
            }

            public void VirtualMethod_In(in T value)
            {
                if (IsNull(value))
                {
                    throw new Exception("shouldn't get here");
                }
            }

            public virtual bool IsNull(T value) => throw new Exception("shouldn't be called");
        }

        public sealed class NullableConverter<T> : JsonConverter<T?> where T : struct
        {
            public override bool IsNull(T? value) => !value.HasValue;
        }
    }

with results:

EqualsOperator 528
EqualsOperator_In 4015
IsOperator 720
IsOperator_In 4008
EqualsMethod 554
EqualsMethod_In 210
VirtualMethod 652
VirtualMethod_In 552

@CodeBlanch
Copy link
Contributor Author

@steveharter I did some benchmarking and updated based on the results. Added the in and CanBeNull check as you suggested (thanks) and also moved the !HandleNullOnWrite check up in the order so the most expensive op (IsNull call) is last.

Benchmark code
	[MemoryDiagnoser]
	public class JsonBenchmarks
	{
		private readonly NullableConverter<int> _NullableConverter = new NullableConverter<int>();
		private readonly CustomValueTypeConverter _CustomValueTypeConverter = new CustomValueTypeConverter();

		[Benchmark]
		public void Nullable_IsNullAsWritten()
		{
			int? value = 1;

			if (_NullableConverter.IsNull(value))
			{
				throw new InvalidOperationException();
			}
		}

		[Benchmark]
		public void Nullable_IsNullWithIn()
		{
			int? value = 1;

			if (_NullableConverter.IsNullWithIn(value))
			{
				throw new InvalidOperationException();
			}
		}

		[Benchmark]
		public void Nullable_IsNullWithInAndCanBeNull()
		{
			int? value = 1;

			if (_NullableConverter.CanBeNull && _NullableConverter.IsNullWithIn(value))
			{
				throw new InvalidOperationException();
			}
		}

		[Benchmark]
		public void CustomValueType_IsNullAsWritten()
		{
			CustomValueType value = default;

			if (_CustomValueTypeConverter.IsNull(value))
			{
				throw new InvalidOperationException();
			}
		}

		[Benchmark]
		public void CustomValueType_IsNullWithIn()
		{
			CustomValueType value = default;

			if (_CustomValueTypeConverter.IsNullWithIn(value))
			{
				throw new InvalidOperationException();
			}
		}

		[Benchmark]
		public void CustomValueType_IsNullWithInAndCanBeNull()
		{
			CustomValueType value = default;

			if (_CustomValueTypeConverter.CanBeNull && _CustomValueTypeConverter.IsNullWithIn(value))
			{
				throw new InvalidOperationException();
			}
		}

		public struct CustomValueType
		{
			public int Integer32 { get; set; }

			public long Integer64 { get; set; }

			public float Float { get; set; }

			public Decimal Decimal { get; set; }

			public string String { get; set; }
		}

		public class NullableConverter<T> : JsonConverter<T?>
			where T : struct
		{
			public override bool IsNull(T? value)
				=> !value.HasValue;

			public override bool IsNullWithIn(in T? value)
				=> !value.HasValue;

			public NullableConverter()
				: base(canBeNull: true)
			{
			}
		}

		public class CustomValueTypeConverter : JsonConverter<CustomValueType>
		{
			public CustomValueTypeConverter()
				: base(canBeNull: false)
			{
			}
		}

		public abstract class JsonConverter<T>
		{
			public bool CanBeNull { get; }

			protected JsonConverter(bool canBeNull)
			{
				CanBeNull = canBeNull;
			}

			public virtual bool IsNull(T value)
				=> value == null;

			public virtual bool IsNullWithIn(in T value)
				=> value == null;
		}
	}
Method Mean Error StdDev Median Gen 0 Gen 1 Gen 2 Allocated
Nullable_IsNullAsWritten 4.8153 ns 0.0279 ns 0.0261 ns 4.8154 ns - - - -
Nullable_IsNullWithIn 1.3472 ns 0.0300 ns 0.0266 ns 1.3506 ns - - - -
Nullable_IsNullWithInAndCanBeNull 1.3561 ns 0.0279 ns 0.0247 ns 1.3562 ns - - - -
CustomValueType_IsNullAsWritten 2.2116 ns 0.2136 ns 0.6164 ns 2.0076 ns - - - -
CustomValueType_IsNullWithIn 1.4693 ns 0.0382 ns 0.0392 ns 1.4648 ns - - - -
CustomValueType_IsNullWithInAndCanBeNull 0.4880 ns 0.0359 ns 0.0384 ns 0.4785 ns - - - -

Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@steveharter steveharter added the tenet-performance Performance related issue label Jan 15, 2021
@steveharter steveharter merged commit f3ec6dc into dotnet:master Jan 15, 2021
@CodeBlanch CodeBlanch deleted the json-perf-regression branch January 15, 2021 17:22
@ghost ghost locked as resolved and limited conversation to collaborators Feb 14, 2021
@steveharter
Copy link
Member

Note that I re-tested these with the latest runtime and the numbers look much different:

EqualsOperator 96
EqualsOperator 96
EqualsOperator_In 96
IsOperator 96
IsOperator_In 96
EqualsMethod 95
EqualsMethod_In 273
VirtualMethod 537
VirtualMethod_In 302

Basically, the "is" operator is now fast. It still emits a "box" opcode, but during JIT that goes away.

I may re-address this with other perf work in #56993.

@eiriktsarpalis
Copy link
Member

I stumbled on the same perf issue when working on the polymorphism prototype. Note that the boxing behavior does not manifest in microbenchmarks, I could only reproduce in the context of the large TryWrite method.

@stephentoub
Copy link
Member

Note that I re-tested these with the latest runtime and the numbers look much different

Only skimming the issue, seems likely this was hitting up against #50915, which has been partially addressed for .NET 6 in #50997.
cc: @EgorBo

@eiriktsarpalis
Copy link
Member

FWIW this particular behavior did reproduce for me long after #50997 got merged.

@steveharter
Copy link
Member

Note that the boxing behavior does not manifest in microbenchmarks, I could only reproduce in the context of the large TryWrite method.

After running the STJ benchmarks, I came across this as well. With a IsNull()-to-is null change in a modified STJ, there was a regression in a POCO that contains Nullable<T> properties , however the microbenchmarks I wrote were fast. It appears the optimization in #50997 is doesn't get applied to larger methods like JsonSerializer.TryWrite<T>(...).

So I will keep the existing IsNull() virtual method for now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants