dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.13k stars 4.7k forks source link

EqualityComparer<TEnum?>.Default allocates #67842

Closed ltrzesniewski closed 2 years ago

ltrzesniewski commented 2 years ago

Description

EqualityComparer<TEnum?>.Default (when T is a nullable enum) is resolved to ObjectEqualityComparer<TEnum?>, which boxes the enum when performing equality comparison. This causes an unnecessary allocation.

Here is a reproduction:

using System;
using System.Collections.Generic;
using System.Runtime.InteropServices;

namespace ConsoleApp;

public static class Program
{
    public static void Main()
    {
        _ = EqualityComparer<DayOfWeek?>.Default.Equals(DayOfWeek.Friday, DayOfWeek.Friday); // Warm-up

        var before = GC.GetAllocatedBytesForCurrentThread();
        _ = EqualityComparer<DayOfWeek?>.Default.Equals(DayOfWeek.Friday, DayOfWeek.Friday);
        var after = GC.GetAllocatedBytesForCurrentThread();

        Console.WriteLine(RuntimeInformation.FrameworkDescription);
        Console.WriteLine(after > before ? "Allocates" : "Allocation-free");
    }
}

This results in:

.NET 7.0.0-preview.2.22152.2
Allocates

If you replace DayOfWeek? with DayOfWeek, or if you use a type such as int? instead, the code will print Allocation-free.

Configuration

I reproduced this on .NET 6 and on .NET 7 preview 2 on Windows x64, but this should be independent of the OS and architecture.

Regression?

This is not a regression. It also reproduces on .NET Framework 4.8.

Analysis

This is roughly due to the fact that ComparerHelpers.CreateDefaultEqualityComparer special-cases both Nullable<T> and enums, but does not special-case nullable enums. This code path ends up using ObjectEqualityComparer<T>.

I'm willing to provide a PR to fix this if you'd like.

dotnet-issue-labeler[bot] commented 2 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

ghost commented 2 years ago

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

Issue Details
### Description `EqualityComparer.Default` (when `T` is a *nullable enum*) is resolved to `ObjectEqualityComparer`, which boxes the enum when performing equality comparison. This causes an unnecessary allocation. Here is a reproduction: ```C# using System; using System.Collections.Generic; using System.Runtime.InteropServices; namespace ConsoleApp; public static class Program { public static void Main() { _ = EqualityComparer.Default.Equals(DayOfWeek.Friday, DayOfWeek.Friday); // Warm-up var before = GC.GetAllocatedBytesForCurrentThread(); _ = EqualityComparer.Default.Equals(DayOfWeek.Friday, DayOfWeek.Friday); var after = GC.GetAllocatedBytesForCurrentThread(); Console.WriteLine(RuntimeInformation.FrameworkDescription); Console.WriteLine(after > before ? "Allocates" : "Allocation-free"); } } ``` This results in: ``` .NET 7.0.0-preview.2.22152.2 Allocates ``` If you replace `DayOfWeek?` with `DayOfWeek`, or if you use a type such as `int?` instead, the code will print `Allocation-free`. ### Configuration I reproduced this on .NET 6 and on .NET 7 preview 2 on Windows x64, but this should be independent of the OS and architecture. ### Regression? This is not a regression. It also reproduces on .NET Framework 4.8. ### Analysis This is roughly due to the fact that [`ComparerHelpers.CreateDefaultEqualityComparer`](https://github.com/dotnet/runtime/blob/7bfc61b970e28f94782ef7c0cbcbbbc94ef9f5eb/src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs#L116) special-cases both `Nullable` *and* enums, but does not special-case nullable enums. This code path ends up using `ObjectEqualityComparer`. I'm willing to provide a PR to fix this if you'd like.
Author: ltrzesniewski
Assignees: -
Labels: `area-System.Collections`, `tenet-performance`, `untriaged`
Milestone: -
EgorBo commented 2 years ago

I'll take a look, the problem that getDefaultComparerClassHelper currently only devirtualizes Nullable<> for T : IEquatable<T> https://github.com/dotnet/runtime/blob/7bfc61b970e28f94782ef7c0cbcbbbc94ef9f5eb/src/coreclr/vm/jitinterface.cpp#L8822-L8827

so we might want to introduce a NullableEnumComparer

ghost commented 2 years ago

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

Issue Details
### Description `EqualityComparer.Default` (when `T` is a *nullable enum*) is resolved to `ObjectEqualityComparer`, which boxes the enum when performing equality comparison. This causes an unnecessary allocation. Here is a reproduction: ```C# using System; using System.Collections.Generic; using System.Runtime.InteropServices; namespace ConsoleApp; public static class Program { public static void Main() { _ = EqualityComparer.Default.Equals(DayOfWeek.Friday, DayOfWeek.Friday); // Warm-up var before = GC.GetAllocatedBytesForCurrentThread(); _ = EqualityComparer.Default.Equals(DayOfWeek.Friday, DayOfWeek.Friday); var after = GC.GetAllocatedBytesForCurrentThread(); Console.WriteLine(RuntimeInformation.FrameworkDescription); Console.WriteLine(after > before ? "Allocates" : "Allocation-free"); } } ``` This results in: ``` .NET 7.0.0-preview.2.22152.2 Allocates ``` If you replace `DayOfWeek?` with `DayOfWeek`, or if you use a type such as `int?` instead, the code will print `Allocation-free`. ### Configuration I reproduced this on .NET 6 and on .NET 7 preview 2 on Windows x64, but this should be independent of the OS and architecture. ### Regression? This is not a regression. It also reproduces on .NET Framework 4.8. ### Analysis This is roughly due to the fact that [`ComparerHelpers.CreateDefaultEqualityComparer`](https://github.com/dotnet/runtime/blob/7bfc61b970e28f94782ef7c0cbcbbbc94ef9f5eb/src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs#L116) special-cases both `Nullable` *and* enums, but does not special-case nullable enums. This code path ends up using `ObjectEqualityComparer`. I'm willing to provide a PR to fix this if you'd like.
Author: ltrzesniewski
Assignees: -
Labels: `tenet-performance`, `area-CodeGen-coreclr`
Milestone: -
ltrzesniewski commented 2 years ago

Thanks @EgorBo! 🙂