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

Reimplement a few helpers with new RuntimeHelpers APIs #100846

Merged
merged 5 commits into from
Apr 11, 2024

Conversation

jkoritzinsky
Copy link
Member

Reimplement InternalBoxEnum and AllocateValueType with the new RuntimeHelpers APIs (SizeOf and Box). This removes some HelperMethodFrames on CoreCLR and shares more code across the runtimes.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 10, 2024
@jkoritzinsky jkoritzinsky added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 10, 2024
rawData = rawData.Slice(sizeof(long) - size);
}

return RuntimeHelpers.Box(ref MemoryMarshal.GetReference(rawData), type)!;
Copy link
Member

Choose a reason for hiding this comment

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

This does some extra work (e.g. argument validation) compared to current state. Is it a measurable perf regression?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wrote some basic benchmarks for Enum.ToObject and actually saw an improvement.

My benchmarks:

    private enum E
    {
        A,
        B,
        C
    }

    [Benchmark]
    public object ToObjectSameSize()
    {
        return Enum.ToObject(typeof(E), 1);
    }

    [Benchmark]
    public object ToObjectLarger()
    {
        return Enum.ToObject(typeof(E), 1L);
    }

    [Benchmark]
    public object ToObjectBoxed()
    {
        return Enum.ToObject(typeof(E), (object)1u);
    }

Results:


BenchmarkDotNet v0.13.12, Windows 10 (10.0.19045.4291/22H2/2022Update)
Intel Core i7-7700 CPU 3.60GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET SDK 9.0.100-preview.2.24157.14
  [Host]     : .NET 7.0.5 (7.0.523.17405), X64 RyuJIT AVX2
  Job-BRSDCR : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2
  Job-ASHIII : .NET 9.0.0 (9.0.24.12805), X64 RyuJIT AVX2


Method Runtime Mean Error StdDev Ratio RatioSD
ToObjectSameSize CoreRun 37.40 ns 0.809 ns 1.480 ns 0.90 0.03
ToObjectSameSize .NET 9.0 42.90 ns 0.170 ns 0.142 ns 1.00 0.00
ToObjectLarger CoreRun 38.20 ns 0.459 ns 0.407 ns 0.89 0.01
ToObjectLarger .NET 9.0 43.12 ns 0.385 ns 0.341 ns 1.00 0.00
ToObjectBoxed CoreRun 40.88 ns 0.127 ns 0.106 ns 0.80 0.01
ToObjectBoxed .NET 9.0 51.13 ns 0.874 ns 0.774 ns 1.00 0.00

Co-authored-by: Aleksey Kliger (λgeek) <akliger@gmail.com>
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@jkoritzinsky jkoritzinsky merged commit b9cd89a into dotnet:main Apr 11, 2024
159 of 163 checks passed
@jkoritzinsky jkoritzinsky deleted the helper-method-frame-box branch April 11, 2024 22:48
@LoopedBard3
Copy link
Member

Potential regression: #101134
@jkoritzinsky

@matouskozak
Copy link
Member

matouskozak commented Apr 16, 2024

Possible mono regressions:
Linux/arm64 AOT-llvm: #104777
Linux/x64 AOT-llvm: dotnet/perf-autofiling-issues#33090, dotnet/perf-autofiling-issues#33089

@jkoritzinsky
Copy link
Member Author

These regressions are all on the path to hit AllocateValueType. I'll take a look.

@jkotas
Copy link
Member

jkotas commented Apr 16, 2024

I have moved the issue to dotnet/runtime: #101134

matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants