Devirtualized default value comparison

[YohDeadfall]

Since there is dotnet/coreclr#14125 made with love by Andy it's possible to inline default value comparison, but this requires direct calls to EqualityComparer<T>.Default followed by Equals. This change removes storing the default comparer in a field and forces devirtualization for value types. As a plus it removes the default value field to avoid unnecessary memory reads and for better optimizations by the JIT.

/cc @stephentoub

[stephentoub]

Can you share benchmarks... including for both T as a value type and as a reference type? Thanks.

[AndyAyersMS]

I think we may handle these cases better now that we did back in the time of dotnet/coreclr#14125 as the late devirtualizer is a bit more capable and we're also able to propagate types from readonly statics once we've hit Tier1.

So I'm also curious to see if these change lead to significant perf improvements.

[YohDeadfall]

Okay, will benchmark this and post back.

Bonus point of the change I realized today:

• It doesn't allocate a comparer just to hold a reference to it in case of reference types. There is a branch only for value types and only it uses the comparer.
• It doesn't hold a zero initiated value which can be inlined by the JIT.

[stephentoub]

Regardless of the rest of the change, there's definitely no need for this to exist.

[steveharter]

@layomia was this intended to actually contain a non-default value at some point in the future (when that feature is added)? e.g. System.String may want a default value of "" instead of null.

However, if\when we allow specifying a default value, we'd want an instance variable on this class (and\or on JsonClassInfo), not a static.

[layomia]

was this intended to actually contain a non-default value at some point in the future (when that feature is added)?

Yup. No need to add it until we have that support - #36236.

we'd want an instance variable on this class (and\or on JsonClassInfo), not a static.

👍

[YohDeadfall]

I see, but if this should be done for strings which is a ref type, then the comparer should be used in both branches, or they must be merged.

[steveharter]

LGTM - I'll let @layomia do the approval.

I don't think this will measurably affect the existing serialization benchmarks so I believe you'll need a standalone repro \ simulation of the before and after patterns.

[YohDeadfall]

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.18363.778 (1909/November2018Update/19H2)
Intel Xeon CPU E5-2687W 0 3.10GHz, 2 CPU, 32 logical and 16 physical cores
.NET Core SDK=5.0.100-preview.6.20302.1
  [Host]        : .NET Core 3.1.4 (CoreCLR 4.700.20.20201, CoreFX 4.700.20.22101), X64 RyuJIT
  .NET Core 5.0 : .NET Core 5.0.0 (CoreCLR 5.0.20.28106, CoreFX 5.0.20.28106), X64 RyuJIT

Int32Benchmark

Method	value	Mean	Error	StdDev	Code Size
IsDefaultInlined	0	1.431 ns	0.0505 ns	0.0448 ns	46 B
IsDefaultStored	0	2.011 ns	0.0043 ns	0.0040 ns	44 B
IsDefaultInlined	2	1.454 ns	0.0010 ns	0.0009 ns	46 B
IsDefaultStored	2	2.044 ns	0.0021 ns	0.0020 ns	44 B

Int64Benchmark

Method	value	Mean	Error	StdDev	Code Size
IsDefaultInlined	0	1.479 ns	0.0011 ns	0.0009 ns	46 B
IsDefaultStored	0	1.929 ns	0.0012 ns	0.0010 ns	44 B
IsDefaultInlined	2	1.528 ns	0.0010 ns	0.0010 ns	46 B
IsDefaultStored	2	1.928 ns	0.0010 ns	0.0008 ns	44 B

SingleBenchmark

Method	value	Mean	Error	StdDev	Code Size
IsDefaultInlined	0	1.902 ns	0.0016 ns	0.0014 ns	83 B
IsDefaultStored	0	3.626 ns	0.0009 ns	0.0008 ns	50 B
IsDefaultInlined	2.7	2.567 ns	0.0895 ns	0.1030 ns	83 B
IsDefaultStored	2.7	3.735 ns	0.0015 ns	0.0014 ns	50 B

DoubleBenchmark

Method	value	Mean	Error	StdDev	Code Size
IsDefaultInlined	0	2.101 ns	0.0567 ns	0.0443 ns	83 B
IsDefaultStored	0	3.539 ns	0.0070 ns	0.0058 ns	50 B
IsDefaultInlined	2.7	3.474 ns	0.0534 ns	0.0474 ns	83 B
IsDefaultStored	2.7	3.651 ns	0.0015 ns	0.0014 ns	50 B

DecimalBenchmark

Method	value	Mean	Error	StdDev	Code Size
IsDefaultInlined	0	2.365 ns	0.0020 ns	0.0017 ns	152 B
IsDefaultStored	0	4.182 ns	0.0021 ns	0.0018 ns	72 B
IsDefaultInlined	2.7	3.708 ns	0.0011 ns	0.0009 ns	152 B
IsDefaultStored	2.7	5.254 ns	0.0014 ns	0.0012 ns	72 B

Code

[SimpleJob(RuntimeMoniker.NetCoreApp50)]
[DisassemblyDiagnoser]
public abstract class Benchmark<T>
{
    private static IEqualityComparer<T> s_defaultComparer = EqualityComparer<T>.Default;

    [Benchmark]
    [ArgumentsSource(nameof(Values))]
    public bool IsDefaultInlined(T value) => EqualityComparer<T>.Default.Equals(default, value);

    [Benchmark]
    [ArgumentsSource(nameof(Values))]
    public bool IsDefaultStored(T value) => s_defaultComparer.Equals(default, value);

    public IEnumerable<object> Values()
    {
        foreach (var value in ValuesCore())
            yield return value;
    }

    protected abstract IEnumerable<T> ValuesCore();
}

public class Int32Benchmark : Benchmark<int>
{
    protected override IEnumerable<int> ValuesCore() => new[] { default, 2 };
}

public class Int64Benchmark : Benchmark<long>
{
    protected override IEnumerable<long> ValuesCore() => new[] { default, 2L };
}

public class SingleBenchmark : Benchmark<float>
{
    protected override IEnumerable<float> ValuesCore() => new[] { default, 2.7f };
}

public class DoubleBenchmark : Benchmark<double>
{
    protected override IEnumerable<double> ValuesCore() => new[] { default, 2.7 };
}

public class DecimalBenchmark : Benchmark<decimal>
{
    protected override IEnumerable<decimal> ValuesCore() => new[] { default, 2.7M };
}

[stephentoub]

Thanks. What about a reference type? That's where I'd expect to see a regression. Or is this json code path never used with a reference type?

[YohDeadfall]

For reference types there is a simple null check on line 105 in the original code:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs

Lines 105 to 107 in fe35942

	if (value == null)
	{
	Debug.Assert(s_defaultValue == null && Converter.CanBeNull);

[stephentoub]

For reference types there is a simple null check on line 105 in the original code:

Yes, but with the way this is currently written, if a reference type comes in and isn't null and IgnoreDefaultValuesOnWrite is true, aren't we still going to try to compare it?

[YohDeadfall]

Oh, yes. There is an issue in the code, will fix it.

[YohDeadfall]

@layomia This one should be changed to the following when you start working on defaults:

DefaultValue is null
    ? value is null // fast check for reference types in case when the default is null
    : EqualityComparer<T>.Default.Equals(DefaultValue, value) // fast for non-shared generics, but slow in other cases

[YohDeadfall]

Why is it named as HandleNull, but not HandlesNull? Is it a short form of ShouldHandleNull?

[layomia]

It should probably be HandlesNull. I'll follow up on this internally.

[stephentoub]

From my perspective, this is now adding complexity that is only worth it if there's a measurable perf benefit when actually using this functionality via the JSON APIs and not just in a microbenchmark on EqualityComparer.

If there's no measurable impact one way or the other, we should do the simplest thing, which is probably to just use EqualityComparer<T>.Default.Equals directly.

[YohDeadfall]

As @steveharter I don't think it will give any visible difference because the serializer suffers from other problems, but it's better than before. Therefore, I'm leaving decision taking to @layomia and the rest of the team, they maintain code mostly and maybe have some plans on further improvement.

[stephentoub]

but it's better than before

How do we know that?

[YohDeadfall]

I mean not the current improvement, but all changes since 3.0.

[layomia]

The condition here is wrong and will cause default to be written when the caller specifies that it should be ignored i.e. JsonIgnoreCondition.WhenWritingDefault is active on the global options or on the property. This is why the tests are failing.

To avoid the EqualityComparer<T>.Default.Equals(default, value) check on reference types that are not null, you can change this line to:

else if (IgnoreDefaultValuesOnWrite && !Converter.CanBeNull && EqualityComparer<T>.Default.Equals(default, value))

The previous logic can be left as-is.

[YohDeadfall]

Found that CanBeNull can be made static because TypeToConvert is sealed and always equals to typeof(T).

[layomia]

The updated condition is also wrong:

• nothing will be written when IgnoreDefaultValuesOnWrite is true and the value is not default.
• default values will be written regardless of the value of IgnoreDefaultValuesOnWrite.
• maybe a couple more things

[layomia]

TLDR: After measuring the perf impact using the JSON APIs, I think the added complexity is worth it.

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19041.329 (2004/?/20H1)
Intel Core i7-7700 CPU 3.60GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=5.0.100-preview.7.20303.12
  [Host]     : .NET Core 5.0.0 (CoreCLR 5.0.20.30301, CoreFX 5.0.20.30301), X64 RyuJIT
  Job-SPWLSQ : .NET Core 5.0 (CoreCLR 42.42.42.42424, CoreFX 42.42.42.42424), X64 RyuJIT

Simplified condition (no fast path for reference types)

Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
Serialize	571.0 ns	13.63 ns	15.15 ns	567.5 ns	555.2 ns	603.9 ns	0.0783	-	-	328 B

Complex condition (fast path for reference types)

Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
Serialize	533.0 ns	8.09 ns	7.57 ns	529.7 ns	520.2 ns	544.4 ns	0.0770	-	-	328 B

summary:
better: 1, geomean: 1.071
total diff: 1

No Slower results for the provided threshold = 0% and noise filter = 0.3ns.

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
System.Text.Json.Serialization.Tests.IgnoreDefaultValues<LargeStructWithProperti	1.07	567.52	529.69

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using BenchmarkDotNet.Attributes;
using MicroBenchmarks;
using MicroBenchmarks.Serializers;

namespace System.Text.Json.Serialization.Tests
{
    [GenericTypeArguments(typeof(LargeStructWithProperties))]
    public class IgnoreDefaultValues<T>
    {
        private T _value;
        private JsonSerializerOptions _options;

        [GlobalSetup]
        public void Setup()
        {
            _value = DataGenerator.Generate<T>();
            _options = new JsonSerializerOptions { DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingDefault };
        }

        [BenchmarkCategory(Categories.Libraries, Categories.JSON)]
        [Benchmark]
        public byte[] Serialize() => JsonSerializer.SerializeToUtf8Bytes(_value, _options);
    }

    public struct LargeStructWithProperties
    {
        public string String1 { get; set; }
        public string String2 { get; set; }
        public string String3 { get; set; }
        public string String4 { get; set; }
        public string String5 { get; set; }
        public int Int1 { get; set; }
        public int Int2 { get; set; }
        public int Int3 { get; set; }
        public int Int4 { get; set; }
        public int Int5 { get; set; }
    }
}

[stephentoub]

Thanks, @layomia. In your repro, all of the strings are null. What results do you get if they're not?

[layomia]

Actually, that's the case where the strings are not null

private static LargeStructWithProperties CreateLargeStructWithProperties()
  => new LargeStructWithProperties
  {
      String1 = "1",
      String2 = "2",
      String3 = "3",
      String4 = "4",
      String5 = "5",
  };

---

Here are the numbers when they are null (which is what I meant to measure above :) -

Simplified condition (no fast path for reference types)

Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
Serialize	303.8 ns	4.52 ns	4.22 ns	302.7 ns	298.2 ns	312.4 ns	0.0625	-	-	264 B

Complex condition (fast path for reference types)

Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
Serialize	253.8 ns	3.66 ns	3.42 ns	253.0 ns	248.5 ns	260.0 ns	0.0620	-	-	264 B

summary:
better: 1, geomean: 1.196
total diff: 1

No Slower results for the provided threshold = 0% and noise filter = 0.3ns.

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
System.Text.Json.Serialization.Tests.IgnoreDefaultValues<LargeStructWithProperti	1.20	302.67	253.00

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using BenchmarkDotNet.Attributes;
using MicroBenchmarks;
using MicroBenchmarks.Serializers;

namespace System.Text.Json.Serialization.Tests
{
    [GenericTypeArguments(typeof(LargeStructWithProperties))]
    public class IgnoreDefaultValues<T>
    {
        private T _value;
        private JsonSerializerOptions _options;

        [GlobalSetup]
        public void Setup()
        {
            _value = default;
            _options = new JsonSerializerOptions { DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingDefault };
        }

        [BenchmarkCategory(Categories.Libraries, Categories.JSON)]
        [Benchmark]
        public byte[] Serialize() => JsonSerializer.SerializeToUtf8Bytes(_value, _options);
    }
}

[layomia]

LGTM. Can merge after last couple of comments are addressed and merge conflicts are fixed.

[layomia]

Thanks.