Skip to content

[Jiterpreter] Add support for TryGetHashCode intrinsic #81644

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

Merged
merged 1 commit into from
Feb 7, 2023

Conversation

AustinWise
Copy link
Contributor

@AustinWise AustinWise commented Feb 4, 2023

#80520 added support in the Jiterpreter for the intrinsic that underlies RuntimeHelpers.GetHashCode. This PR adds similar support for the intrinsic that underlies RuntimeHelpers.TryGetHashCode.

For reference RuntimeHelpers.TryGetHashCode was added in #80059. It is currently only used in ConditionalWeakTable.TryGetValue.

Testing

./dotnet.sh build /t:Test src/libraries/System.Runtime/tests /p:TargetOS=browser /p:TargetArchitecture=wasm /p:Configuration=Release

@ghost ghost added area-Codegen-Interpreter-mono community-contribution Indicates that the PR has been added by a community member labels Feb 4, 2023
@ghost
Copy link

ghost commented Feb 4, 2023

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

Issue Details

#80520 added support in the Jiterpreter for the intrinsic that underlies RuntimeHelpers.GetHashCode. This PR adds similar support for the intrinsic that underlies RuntimeHelpers.TryGetHashCode.

For reference RuntimeHelpers.TryGetHashCode was added in #80059. It is currently only used in ConditionalWeakTable.TryGetValue.

Author: AustinWise
Assignees: -
Labels:

area-Codegen-Interpreter-mono, community-contribution

Milestone: -

@AustinWise
Copy link
Contributor Author

@kg is there a documented way to test and benchmark Jiterpreter changes? I had BenchmarkDotNet project that defined some custom WASM jobs, but it appears that no longer works.

@kg
Copy link
Member

kg commented Feb 4, 2023

@kg is there a documented way to test and benchmark Jiterpreter changes? I had BenchmarkDotNet project that defined some custom WASM jobs, but it appears that no longer works.

It should work the same as anything else, you can manually adjust the tiering thresholds for it so that traces compile sooner (either by passing a runtime arg, or editing the defaults in options-def.h).

To set a runtime arg you can pass it from JS when initializing the runtime.

@AustinWise
Copy link
Contributor Author

AustinWise commented Feb 5, 2023

I found the incantation to make my benchmark work again:

./dotnet.sh build -p:TargetOS=browser -p:TargetArchitecture=wasm -c Release src/mono/wasm/Wasm.Build.Tests /t:InstallWorkloadUsingArtifacts

This creates the dotnet-latest directory containing a .NET SDK that can be used with BenchmarkDotNet.

Anyways, the results of running the benchmark:

BenchmarkDotNet=v0.13.3, OS=ubuntu 22.04
AMD Ryzen Threadripper PRO 3955WX 16-Cores, 1 CPU, 32 logical and 16 physical cores
.NET SDK=7.0.102
  [Host]     : .NET 7.0.2 (7.0.222.60605), X64 RyuJIT AVX2
  PR         : .NET Core (Mono) 8.0.0-dev, Wasm AOT
  merge-base : .NET Core (Mono) 8.0.0-dev, Wasm AOT

Runtime=Wasm  IterationCount=15  LaunchCount=2  
WarmupCount=10  
V8 version 11.1.92
Method Job Toolchain NumberOfObjects Mean Error StdDev Ratio RatioSD
TryGetNonExistentValue PR Wasm: PR 1000 132.9 ns 0.83 ns 1.21 ns 1.01 0.01
TryGetNonExistentValue merge-base Wasm: merge-base 1000 131.0 ns 1.07 ns 1.56 ns 1.00 0.00
TryGetExistingValue PR Wasm: PR 1000 198.4 ns 2.41 ns 3.61 ns 0.90 0.02
TryGetExistingValue merge-base Wasm: merge-base 1000 219.8 ns 0.96 ns 1.41 ns 1.00 0.00

I don't know for sure if the Jiterpreter is even being run by this benchmark, but it does seem to improve performance. Running it a second time has similar results, so I'm assuming it is not a fluke caused by differing numbers of has collisions run-to-run.

@AustinWise AustinWise marked this pull request as ready for review February 5, 2023 03:54
@kg
Copy link
Member

kg commented Feb 6, 2023

You'll need to rebase this now to resolve a conflict (sorry! 🙇‍♀️), it shouldn't be too hard to do though. The jiterpreter code just got rearranged and split into two files, but copying your work over should be a simple bit of copy-paste.

@AustinWise AustinWise force-pushed the austin/JiterpreterTryGetHashCode branch from 9ba8743 to f1fec31 Compare February 6, 2023 21:44
@AustinWise AustinWise force-pushed the austin/JiterpreterTryGetHashCode branch from f1fec31 to d648642 Compare February 6, 2023 22:14
@AustinWise
Copy link
Contributor Author

I resolved the merge conflicts and the tests pass.

@kg
Copy link
Member

kg commented Feb 7, 2023

I resolved the merge conflicts and the tests pass.

Great! I'll make sure we're cool adding this (I don't see any problems) and get it merged.

@kg kg merged commit 59dddfd into dotnet:main Feb 7, 2023
@kg
Copy link
Member

kg commented Feb 7, 2023

Thank you for your contribution! 🎉

@AustinWise AustinWise deleted the austin/JiterpreterTryGetHashCode branch February 8, 2023 21:48
@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Codegen-Interpreter-mono community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants