NotificationsYou must be signed in to change notification settings
Fork5.2k
Star17.2k

Convert fast path of ValueType.GetHashCode to managed#97590

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

jkotas merged 19 commits intodotnet:mainfromhuoyaoyuan:cancomparebits

Feb 21, 2024

Merged

Convert fast path of ValueType.GetHashCode to managed#97590

jkotas merged 19 commits intodotnet:mainfromhuoyaoyuan:cancomparebits

Feb 21, 2024

Conversation

Copy link

Member

huoyaoyuan commentedJan 27, 2024

Also converts HELPER_METHOD_FRAME for slow path to QCall.

huoyaoyuan added9 commits

January 27, 2024 17:02

Access CanCompareBits from MethodTableAuxiliaryData

ade6628

Switch to QCall

246168c

Implement the QCall

580f969

Fast path of GetHashCode

99b4ad9

Reduce MethodTable access

6fa4666

Expose GetTypeID

6d54f40

Call for RegularGetValueTypeHashCode

5a5da06

Cleanup unused FCall

ce7f636

Use HashCode type

7091fb0

ghost added area-VM-coreclr community-contributionIndicates that the PR has been added by a community member labels

Jan 27, 2024

huoyaoyuan commented

Jan 27, 2024

View reviewed changes

...coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs


		// Subset of src\vm\methodtable.h
		[StructLayout(LayoutKind.Explicit)]
		internal unsafestruct MethodTableAuxiliaryData

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

I'm not sure if this is the desired direction to expose this, but it's relatively simple.

src/coreclr/System.Private.CoreLib/src/System/ValueType.cs Outdated

Comment on lines 107 to 109

		// We don't want to expose the method table pointer in the hash code
		// Let's use the typeID instead.
		uinttypeID=RuntimeHelpers.GetTypeID(pMT);

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Is this something we still care about? I assumeGetType().GetHashCode() would be slower.

Copy link

Member

Choose a reason for hiding this comment

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

I think running the methodtable pointer throughHashCode would be good enough. If you do that, the multiplication on the next won't be necessary.

src/coreclr/System.Private.CoreLib/src/System/ValueType.cs Outdated

		hashCode^=RegularGetValueTypeHashCode(pMT,ObjectHandleOnStack.Create(refobj));
		}

		GC.KeepAlive(this);

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Should be unnecessary

Copy link

Member

Choose a reason for hiding this comment

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

Right

src/coreclr/System.Private.CoreLib/src/System/ValueType.cs Outdated

		}

		[LibraryImport(RuntimeHelpers.QCall,EntryPoint="ValueType_RegularGetValueTypeHashCode")]
		[SuppressGCTransition]

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

The unmanaged slow path manipulates the object reference in COOP mode

Copy link

Member

Choose a reason for hiding this comment

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

It can also throw and call back into managed code. It is not ok to use SuppressGCTransition with that.

SuppressGCTransition can be only used for leaf methods.

Copy link

Member

Choose a reason for hiding this comment

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

If you would like to make it more managed, you can change the helper to return the strategy to use to compute the hashcode:

Call object.GetHashCode for object reference at given offset
Call double.GetHashCode for field at given offset
Call float.GetHashCode for field at given offset
Hash N bytes from given offset (this can also cover the case whereCanCompareBitsOrUseFastGetHashCode was not computed)

Copy link

MemberAuthor

huoyaoyuanJan 27, 2024•
edited
Loading

Choose a reason for hiding this comment

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

Got some chanllenge when handling the recursive case.
How to pass a reference pointing to the middle of an object to QCall? There's a leaf methodGetFieldTypeHandleThrowing marked as throwing, so it looks unsuitable for FCall since we are moving away from HELPER_METHOD_FRAME.
Or, will it really throw when loading a field handle of already boxed type? Is there any alternate?

Copy link

MemberAuthor

huoyaoyuanJan 27, 2024

Choose a reason for hiding this comment

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

Well, P/Invoke generator can pin theref byte. Is it reliable/performant for object fields? Can GCPROTECT be omitted then?

Copy link

Member

jkotasJan 27, 2024

Choose a reason for hiding this comment

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

I think it would work better if you return field offset from the QCall and compute the byref on the managed side. Nothing to GC protect, etc.

Copy link

MemberAuthor

huoyaoyuanJan 27, 2024

Choose a reason for hiding this comment

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

For the recursive value type case, it requires a reference pointing to the field, which is not a top level object. Passing object handle+offset does work, but looks a bit confusing if more levels are involved.

Copy link

Member

jkotasJan 27, 2024

Choose a reason for hiding this comment

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

Compound offset like this is not uncommon. It is frequently used e.g. by JITed code.

My preference is to have less manually managed code. Less GC mode switches in VM and less manually managed code that runs in a cooperative mode is goodness.

jkotas requested a review fromAaronRobinsonMSFT

January 27, 2024 13:51

build-analysisbot mentioned this pull request

Jan 27, 2024

DataContractSerializerTests.DCS_MyPersonSurrogate_Stress failing in CI#35066

Open

AaronRobinsonMSFT reviewed

Jan 27, 2024

View reviewed changes

src/coreclr/vm/comutilnative.h OutdatedShow resolvedHide resolved

src/coreclr/vm/comutilnative.cpp OutdatedShow resolvedHide resolved

huoyaoyuan added2 commits

January 28, 2024 00:36

Remove TypeID usage

a0a1e32

Move each branch to managed

808ceb4

Copy link

MemberAuthor

huoyaoyuan commentedJan 28, 2024•
edited
Loading

There is also an inconsistency in old code: if the first field is a struct type that overridesGetHashCode, then the overridden method won't be called by the fallback method. In contrast, the overriddenEquals will get called by fallbackEquals. Consider following code:

structStruct2{publicStruct1field;}structStruct1{publicstring?o;publicoverrideintGetHashCode()=>StringComparer.OrdinalIgnoreCase.GetHashCode(o??"");publicoverrideboolEquals(object?obj)=>objisStruct1other&&string.Equals(o,other.o,StringComparison.OrdinalIgnoreCase);}Struct1a1=new(){o="ABC"},b1=new(){o="abc"};Struct2a2=new(){field=a1},b2=new(){field=b1};Console.WriteLine(a1.Equals(b1));// trueConsole.WriteLine(a1.GetHashCode()==b1.GetHashCode());// trueConsole.WriteLine(a2.Equals(b2));// trueConsole.WriteLine(a2.GetHashCode()==b2.GetHashCode());// false

This can be fixed in follow-up.

huoyaoyuan added2 commits

January 28, 2024 19:18

Handle recursive case inside native code

9a8de4c

Remove Double and Single from CoreLibBinder

83e49b7

build-analysisbot mentioned this pull request

Jan 28, 2024

[8.0] Failing to build native components#94823

Closed

Fix return

4303b3c

jkotas reviewed

Jan 28, 2024

View reviewed changes

src/coreclr/vm/comutilnative.cpp OutdatedShow resolvedHide resolved

jkotas reviewed

Jan 28, 2024

View reviewed changes

src/coreclr/vm/comutilnative.cpp OutdatedShow resolvedHide resolved

jkotas reviewed

Jan 28, 2024

View reviewed changes

src/coreclr/vm/comutilnative.cpp OutdatedShow resolvedHide resolved

jkotas reviewed

Jan 28, 2024

View reviewed changes

src/coreclr/vm/comutilnative.cppShow resolvedHide resolved

jkotas reviewed

Jan 28, 2024

View reviewed changes

src/coreclr/vm/comutilnative.cpp OutdatedShow resolvedHide resolved

jkotas reviewed

Jan 28, 2024

View reviewed changes

src/coreclr/vm/comutilnative.cpp OutdatedShow resolvedHide resolved

jkotas reviewed

Jan 28, 2024

View reviewed changes

src/coreclr/vm/comutilnative.cpp OutdatedShow resolvedHide resolved

jkotas reviewed

Jan 28, 2024

View reviewed changes

src/coreclr/vm/comutilnative.cpp OutdatedShow resolvedHide resolved

Complete

e519b2b

huoyaoyuan force-pushed thecancomparebits branch from4495372 toe519b2bCompare

January 29, 2024 13:54

huoyaoyuan mentioned this pull request

Jan 29, 2024

Convert HELPER_METHOD_FRAME in objectnative.cpp to QCall#97641

Merged

jkotas reviewed

Jan 29, 2024

View reviewed changes

src/coreclr/vm/comutilnative.cpp Outdated

		CorElementType fieldType = field->GetFieldType();
		if (fieldType == ELEMENT_TYPE_R8)
		{
		*fieldOffset = field->GetOffsetUnsafe();

Copy link

Member

jkotasJan 29, 2024

Choose a reason for hiding this comment

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

Suggested change

	*fieldOffset = field->GetOffsetUnsafe();
	*fieldOffset+= field->GetOffsetUnsafe();

I think all these need to be+= to make the recursive case work.

Copy link

Member

jkotasJan 29, 2024

Choose a reason for hiding this comment

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

I may be a good idea to add a test for the recursive case to src\libraries\system.Runtime\tests\System.Runtime.Tests\System\ValueTypeTests.cs that would catch this mistake.

jkotas reviewed

Jan 29, 2024

View reviewed changes

src/coreclr/vm/comutilnative.cppShow resolvedHide resolved

Handle offset of recursive case

9c80cd4

This was referencedJan 31, 2024

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono#82637

Closed

Tests crashing in CI with no dump: exit code 137 means SIGKILL Killed#97049

Closed

[browser][MT] Unhandled JS error#97546

Closed

AaronRobinsonMSFT reviewed

Jan 31, 2024

View reviewed changes

src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ValueTypeTests.cs Outdated

Comment on lines 311 to 312

		obj1.o=null;
		obj1.value.value=2;

Copy link

Member

AaronRobinsonMSFTJan 31, 2024

Choose a reason for hiding this comment

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

Suggested change

	obj1.o=null;
	obj1.value.value=2;
	obj2.o=null;
	obj2.value.value=2;

I assume?

src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ValueTypeTests.cs

		}

		[Fact]
		publicstaticvoidStructContainsPointerNestedCompareTest()

Copy link

Member

AaronRobinsonMSFTJan 31, 2024

Choose a reason for hiding this comment

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

Because these two values are not the same the fact they aren't equal makes sense. We should also have a test where they are equal to validate the other direction.

Copy link

MemberAuthor

huoyaoyuanFeb 20, 2024

Choose a reason for hiding this comment

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

Well all the other tests are only testing the equal case. Returning different hash code is just an implementation detail. The test should be reversed and only test equal case instead.

Copy link

Member

jkotas commentedJan 31, 2024

The new test is failing on native AOT:

[FAIL] System.Tests.ValueTypeTests.StructContainsPointerNestedCompareTestAssert.NotEqual() Failure: Values are equalExpected: Not -413602087Actual:       -413602087   at System.Tests.ValueTypeTests.StructContainsPointerNestedCompareTest() + 0xdc

Copy link

Member

jkotas commentedJan 31, 2024

The new test is failing on native AOT:

If it looks like a native AOT bug, you can file an issue on it and disable the test against it.

build-analysisbot mentioned this pull request

Jan 31, 2024

Assertion failed 'hwintrinsicChild->isContained()' in 'System.Numerics.Tensors.TensorPrimitives+ScaleBOperator#97688

Closed

AaronRobinsonMSFT added the needs-author-actionAn issue or pull request that requires more info or actions from the author. label

Feb 6, 2024

Merge branch 'main' into cancomparebits

9cd9166

Copy link

MemberAuthor

huoyaoyuan commentedFeb 20, 2024•
edited
Loading

If it looks like a native AOT bug, you can file an issue on it and disable the test against it.

I don't think it's actually a bug. Different values may have equal hash code, it's not violating the contract.
The coreclr behavior which reads next field when meetingnull also makes the implementation more complicated.~~I'd like to keep the behavior as-is and limit the test to run on coreclr only.~~

Test equal case only

946967e

Copy link

MemberAuthor

huoyaoyuan commentedFeb 20, 2024

Removing the behavior that skipsnull field can simplify the implementation and remove the need to pass the object itself to native code. Should we change the behavior instead?