Skip to content

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.

Sign up for GitHub

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
jkotas merged 19 commits into from
Feb 21, 2024
Merged

Convert fast path of ValueType.GetHashCode to managed #97590

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
ade6628
Access CanCompareBits from MethodTableAuxiliaryData
huoyaoyuan Jan 27, 2024
246168c
Switch to QCall
huoyaoyuan Jan 27, 2024
580f969
Implement the QCall
huoyaoyuan Jan 27, 2024
99b4ad9
Fast path of GetHashCode
huoyaoyuan Jan 27, 2024
6fa4666
Reduce MethodTable access
huoyaoyuan Jan 27, 2024
6d54f40
Expose GetTypeID
huoyaoyuan Jan 27, 2024
5a5da06
Call for RegularGetValueTypeHashCode
huoyaoyuan Jan 27, 2024
ce7f636
Cleanup unused FCall
huoyaoyuan Jan 27, 2024
7091fb0
Use HashCode type
huoyaoyuan Jan 27, 2024
a0a1e32
Remove TypeID usage
huoyaoyuan Jan 27, 2024
808ceb4
Move each branch to managed
huoyaoyuan Jan 27, 2024
9a8de4c
Handle recursive case inside native code
huoyaoyuan Jan 28, 2024
83e49b7
Remove Double and Single from CoreLibBinder
huoyaoyuan Jan 28, 2024
4303b3c
Fix return
huoyaoyuan Jan 28, 2024
8dd42ff
Apply suggestions from code review
huoyaoyuan Jan 29, 2024
e519b2b
Complete
huoyaoyuan Jan 29, 2024
9c80cd4
Handle offset of recursive case
huoyaoyuan Jan 31, 2024
9cd9166
Merge branch 'main' into cancomparebits
huoyaoyuan Feb 20, 2024
946967e
Test equal case only
huoyaoyuan Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/coreclr/vm/comutilnative.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1732,6 +1732,7 @@ static ValueTypeHashCodeStrategy GetHashCodeStrategy(MethodTable* mt, QCall::Obj
// if we get an object reference we get the hash code out of that
Comment thread
huoyaoyuan marked this conversation as resolved.
if (*(Object**)((BYTE *)objHandle.Get()->UnBox() + *fieldOffset + field->GetOffsetUnsafe()) != NULL)
{
*fieldOffset += field->GetOffsetUnsafe();
ret = ValueTypeHashCodeStrategy::ReferenceField;
Comment thread
huoyaoyuan marked this conversation as resolved.
}
else
Expand All @@ -1745,17 +1746,17 @@ static ValueTypeHashCodeStrategy GetHashCodeStrategy(MethodTable* mt, QCall::Obj
CorElementType fieldType = field->GetFieldType();
if (fieldType == ELEMENT_TYPE_R8)
{
*fieldOffset = field->GetOffsetUnsafe();
*fieldOffset += field->GetOffsetUnsafe();
ret = ValueTypeHashCodeStrategy::DoubleField;
}
else if (fieldType == ELEMENT_TYPE_R4)
{
*fieldOffset = field->GetOffsetUnsafe();
*fieldOffset += field->GetOffsetUnsafe();
ret = ValueTypeHashCodeStrategy::SingleField;
}
else if (fieldType != ELEMENT_TYPE_VALUETYPE)
{
*fieldOffset = field->GetOffsetUnsafe();
*fieldOffset += field->GetOffsetUnsafe();
*fieldSize = field->LoadSize();
ret = ValueTypeHashCodeStrategy::FastGetHashCode;
}
Expand All @@ -1767,7 +1768,7 @@ static ValueTypeHashCodeStrategy GetHashCodeStrategy(MethodTable* mt, QCall::Obj
MethodTable* fieldMT = fieldTH.GetMethodTable();
if (CanCompareBitsOrUseFastGetHashCode(fieldMT))
{
*fieldOffset = field->GetOffsetUnsafe();
*fieldOffset += field->GetOffsetUnsafe();
*fieldSize = field->LoadSize();
ret = ValueTypeHashCodeStrategy::FastGetHashCode;
}
Expand Down
21 changes: 21 additions & 0 deletions src/libraries/System.Runtime/tests/System.Runtime.Tests/System/ValueTypeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,21 @@ public static void StructContainsPointerCompareTest()
Assert.True(obj1.Equals(obj2));
Assert.Equal(obj1.GetHashCode(), obj2.GetHashCode());
}

[Fact]
public static void StructContainsPointerNestedCompareTest()

@AaronRobinsonMSFT AaronRobinsonMSFT Jan 31, 2024

Copy link
Copy Markdown
Member

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.

@huoyaoyuan huoyaoyuan Feb 20, 2024

Copy link
Copy Markdown
Member Author

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.

{
StructContainsPointerNested obj1 = new StructContainsPointerNested();
obj1.o = null;
obj1.value.value = 1;

StructContainsPointerNested obj2 = new StructContainsPointerNested();
obj1.o = null;
obj1.value.value = 2;

@AaronRobinsonMSFT AaronRobinsonMSFT Jan 31, 2024

Copy link
Copy Markdown
Member

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?


Assert.False(obj1.Equals(obj2));
Assert.NotEqual(obj1.GetHashCode(), obj2.GetHashCode());
}

public struct S
{
Expand Down Expand Up @@ -392,5 +407,11 @@ public struct StructContainsPointer
public double value1;
public double value2;
}

public struct StructContainsPointerNested
{
public object o;
public StructNonOverriddenEqualsOrGetHasCode value;
}
}
}