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

EqualityComparer<TEnum?>.Default allocates #67842

Closed
ltrzesniewski opened this issue Apr 11, 2022 · 5 comments · Fixed by #68077
Closed

EqualityComparer<TEnum?>.Default allocates #67842

ltrzesniewski opened this issue Apr 11, 2022 · 5 comments · Fixed by #68077
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI tenet-performance Performance related issue
Milestone

Comments

@ltrzesniewski
Copy link
Contributor

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.

@ltrzesniewski ltrzesniewski added the tenet-performance Performance related issue label Apr 11, 2022
@dotnet-issue-labeler
Copy link

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.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 11, 2022
@ghost
Copy link

ghost commented Apr 11, 2022

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<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.

Author: ltrzesniewski
Assignees: -
Labels:

area-System.Collections, tenet-performance, untriaged

Milestone: -

@EgorBo
Copy link
Member

EgorBo commented Apr 11, 2022

I'll take a look, the problem that getDefaultComparerClassHelper currently only devirtualizes Nullable<> for T : IEquatable<T>

TypeHandle iequatable = TypeHandle(CoreLibBinder::GetClass(CLASS__IEQUATABLEGENERIC)).Instantiate(nullableInst);
if (nullableInst[0].CanCastTo(iequatable))
{
TypeHandle resultTh = ((TypeHandle)CoreLibBinder::GetClass(CLASS__NULLABLE_COMPARER)).Instantiate(nullableInst);
return CORINFO_CLASS_HANDLE(resultTh.GetMethodTable());
}

so we might want to introduce a NullableEnumComparer

@EgorBo EgorBo added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed area-System.Collections untriaged New issue has not been triaged by the area owner labels Apr 11, 2022
@EgorBo EgorBo added this to the Future milestone Apr 11, 2022
@ghost
Copy link

ghost commented Apr 11, 2022

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

Issue Details

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.

Author: ltrzesniewski
Assignees: -
Labels:

tenet-performance, area-CodeGen-coreclr

Milestone: -

@EgorBo EgorBo self-assigned this Apr 11, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 15, 2022
@EgorBo EgorBo modified the milestones: Future, 7.0.0 Apr 15, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 31, 2022
@ltrzesniewski
Copy link
Contributor Author

Thanks @EgorBo! 🙂

@ghost ghost locked as resolved and limited conversation to collaborators Jul 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants