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

FastMod for EEHashTable (Faster virtual generics) #65926

Merged
merged 5 commits into from
Mar 1, 2022

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Feb 27, 2022

Closes #65778

It was suggested to use a different data structure for CORINFO_HELP_VIRTUAL_FUNC_PTR but that is a more complicated change and I think this one worth having too anyway.

Benchmark:

using System.Text.Json.Nodes;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkSwitcher.FromAssembly(typeof(Benchmarks).Assembly).Run(args);

public class Benchmarks
{
    public virtual void VGM<T>() { }

    [Benchmark]
    public void CallVirtualGenericMethod() => VGM<int>();



    // System.Text.Json
    JsonNode _jsonNode = true;

    [Benchmark]
    public bool JsonNodeConversion() => (bool)_jsonNode;



    // string interning
    string str = "Hello";

    [Benchmark]
    public string StrIsInterned() => string.IsInterned(str);
}
Method Toolchain Mean Error StdDev Ratio
CallVirtualGenericMethod \Core_Root\corerun.exe 5.299 ns 0.0364 ns 0.0322 ns 1.00
CallVirtualGenericMethod \Core_Root_PR\corerun.exe 4.166 ns 0.1064 ns 0.1383 ns 0.79
JsonNodeConversion \Core_Root\corerun.exe 5.509 ns 0.0070 ns 0.0062 ns 1.00
JsonNodeConversion \Core_Root_PR\corerun.exe 4.177 ns 0.0461 ns 0.0431 ns 0.76
StrIsInterned \Core_Root\corerun.exe 54.741 ns 0.1900 ns 0.1780 ns 1.00
StrIsInterned \Core_Root_PR\corerun.exe 50.172 ns 0.3521 ns 0.2750 ns 0.92

@EgorBo
Copy link
Member Author

EgorBo commented Feb 27, 2022

PTAL @VSadov @jkotas

@VSadov
Copy link
Member

VSadov commented Feb 27, 2022

In the long term we should experiment with a more lightweight caching scheme for virtual generics.
However, this change is an obvious improvement to the current implementation and will benefit everything that uses EEHashTable.

Copy link
Member

@VSadov VSadov 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!!

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.

Thanks!

@EgorBo EgorBo merged commit af71404 into dotnet:main Mar 1, 2022
@elinor-fung
Copy link
Member

@EgorBo System.Runtime.InteropServices.Tests.GetEndComSlotTests.GetEndComSlot_Windows_ReturnsExpected are failing on x64 with this change: https://dev.azure.com/dnceng/public/_build/results?buildId=1634854&view=ms.vss-test-web.build-test-results-tab&runId=45284702&resultId=151347&paneView=debug

System.Runtime.InteropServices.Tests.GetEndComSlotTests.GetEndComSlot_Windows_ReturnsExpected(type: typeof(System.Runtime.InteropServices.Tests.Common.IComImportObject), expected: 6) [FAIL]
  System.DivideByZeroException : Attempted to divide by zero.
  Stack Trace:
       at System.Runtime.InteropServices.Marshal.GetEndComSlot(Type t)

@EgorBo
Copy link
Member Author

EgorBo commented Mar 1, 2022

Ouch, sorry for that, I thought that job failed everywhere and merged, looking at it now
Apparently, I need to check for zero when I calculate the new multiplier

@EgorBo
Copy link
Member Author

EgorBo commented Mar 1, 2022

@elinor-fung filed #66035

@EgorBo
Copy link
Member Author

EgorBo commented Mar 17, 2022

Improvement on alpine-x64 dotnet/perf-autofiling-issues#3977

@ghost ghost locked as resolved and limited conversation to collaborators Apr 16, 2022
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.

Use Fast-Mod algorithm for native EEHashTableBase
4 participants