Move GetHashCode nullcheck to managed

[MichalPetryka]

Should make it cheaper for GetHashCode on null due to not doublechecking and fcalling while keeping same perf (or better due to JIT folding away the check) for non null.

[dotnet-policy-service]

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

[MichalPetryka]

@MihuBot -nuget

[MichalPetryka]

@MihuBot -nuget

[MichalPetryka]

@jkotas do we want this inlined or not? it helps to optimize out the check but pollutes code when we can't

[jkotas]

I do not think this PR is an improvement. It makes the generated code bigger to optimize rare case. I do not think it is common for this method to be called on null.

[MichalPetryka]

I do not think this PR is an improvement. It makes the generated code bigger to optimize rare case. I do not think it is common for this method to be called on null.

The check was already there in unmanaged code so the total code isn't bigger.

[jkotas]

The aggressively inlined null check is going to make every callsite bigger

[jkotas]

This will make the method less likely to be inclined by the JIT, and when it gets inlined, the inlined code is going to be bigger. Both of these can manifest as regressions.

[MichalPetryka]

This will make the method less likely to be inclined by the JIT, and when it gets inlined, the inlined code is going to be bigger. Both of these can manifest as regressions.

Yeah but when inlining the check can be eliminated, wheras otherwise it would always execute before.

[jkotas]

These claims need numbers.

[MichalPetryka]

@EgorBot -amd -arm

using BenchmarkDotNet.Attributes;
using System.Runtime.CompilerServices;

namespace Bench;

public class Benchmark
{
    private static readonly object? o1 = new();
    private static readonly object? o2 = null;
    private readonly object? o3 = new();
    private readonly object? o4 = null;

    [Benchmark]
    public int KnownNotNull() => RuntimeHelpers.GetHashCode(o1);

    [Benchmark]
    public int KnownNull() => RuntimeHelpers.GetHashCode(o2);

    [Benchmark]
    public int UnknownNotNull() => RuntimeHelpers.GetHashCode(o3);

    [Benchmark]
    public int UnknownNull() => RuntimeHelpers.GetHashCode(o4);
}

[MichalPetryka]

@EgorBot -amd -arm

using BenchmarkDotNet.Attributes;
using System.Runtime.CompilerServices;

namespace Bench;

public class Benchmark
{
    private static readonly int[] arr = new int[10000];

    private static readonly object? o1 = new();
    private static readonly object? o2 = null;
    private readonly object? o3 = new();
    private readonly object? o4 = null;

    [Benchmark]
    public int[] KnownNotNull() => Hash(o1);

    [Benchmark]
    public int[] KnownNull() => Hash(o2);

    [Benchmark]
    public int[] UnknownNotNull() => Hash(o3);

    [Benchmark]
    public int[] UnknownNull() => Hash(o4);

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    private static int[] Hash(object? o)
    {
        int[] a = arr;
        for (int i = 0; i < a.Length; i++)
        {
            a[i] = RuntimeHelpers.GetHashCode(o);
        }

        return a;
    }
}

[MichalPetryka]

Hmm perf for non null is the same on x64 but worse on arm64, weird.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "0ec6ca6b1438bc0e2329e8d910bb8ae516b878f4",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "1286e561578886fd46ca631ee87f80ced8aa7454",
  "last_reviewed_commit": "0ec6ca6b1438bc0e2329e8d910bb8ae516b878f4",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "1286e561578886fd46ca631ee87f80ced8aa7454",
  "last_recorded_worker_run_id": "29682987635",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "0ec6ca6b1438bc0e2329e8d910bb8ae516b878f4",
      "review_id": 4730585669
    }
  ]
}

[github-actions]

Holistic Review

Motivation: RuntimeHelpers.GetHashCode(object?) on CoreCLR previously performed the null check inside the native FCall TryGetHashCode and again in the managed GetHashCodeWorker slow path. For the common non-null case this meant an unavoidable FCall transition plus a redundant null check, and for the null case it paid the cost of an FCall just to return 0. The goal is to make the null case cheaper and let the JIT fold away the check when the argument is provably non-null.

Approach: The managed GetHashCode now checks o is null up front and returns 0 directly, before ever calling into the runtime. The TryGetHashCode FCall parameter is changed from object? to object, and the native implementation in comutilnative.cpp replaces its runtime null check with _ASSERTE(obj != NULL). The now-redundant null check is removed from the GetHashCodeWorker local function. This preserves the documented contract (0 for null) while removing duplicate checks and enabling JIT folding on non-null inputs.

Summary: This is a small, well-targeted micro-optimization that is functionally equivalent to the prior behavior. I verified the only two managed callers of TryGetHashCode are safe with the new non-null contract: GetHashCode guards with the new o is null early return before calling it, and ConditionalWeakTable.FindEntry already asserts and validates key as non-null. The _ASSERTE correctly documents the tightened precondition and will catch contract violations in checked builds without adding release-build overhead. The change is consistent with how NativeAOT already declares TryGetHashCode(object o) as non-nullable. No behavioral change, no public API impact, and no test changes are warranted since existing GetHashCode coverage (including the null case) exercises this path. LGTM.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 41.7 AIC · ⌖ 14.5 AIC · ⊞ 10K