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

Convert fast path of ValueType.GetHashCode to managed #97590

Merged
merged 19 commits into from
Feb 21, 2024

Conversation

huoyaoyuan
Copy link
Member

Also converts HELPER_METHOD_FRAME for slow path to QCall.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 27, 2024
@@ -615,6 +630,28 @@ public TypeHandle GetArrayElementTypeHandle()
public extern uint GetNumInstanceFieldBytes();
}

// Subset of src\vm\methodtable.h
[StructLayout(LayoutKind.Explicit)]
internal unsafe struct MethodTableAuxiliaryData
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'm not sure if this is the desired direction to expose this, but it's relatively simple.

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.
uint typeID = RuntimeHelpers.GetTypeID(pMT);
Copy link
Member Author

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 assume GetType().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 through HashCode would be good enough. If you do that, the multiplication on the next won't be necessary.

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

GC.KeepAlive(this);
Copy link
Member Author

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

}

[LibraryImport(RuntimeHelpers.QCall, EntryPoint = "ValueType_RegularGetValueTypeHashCode")]
[SuppressGCTransition]
Copy link
Member Author

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 where CanCompareBitsOrUseFastGetHashCode was not computed)

Copy link
Member Author

@huoyaoyuan huoyaoyuan Jan 27, 2024

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 method GetFieldTypeHandleThrowing 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
Member Author

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 the ref byte. Is it reliable/performant for object fields? Can GCPROTECT be omitted then?

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 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
Member Author

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

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.

src/coreclr/vm/comutilnative.h Outdated Show resolved Hide resolved
src/coreclr/vm/comutilnative.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/comutilnative.cpp Outdated Show resolved Hide resolved
@huoyaoyuan
Copy link
Member Author

huoyaoyuan commented Jan 28, 2024

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

struct Struct2
{
    public Struct1 field;
}

struct Struct1
{
    public string? o;
    public override int GetHashCode() => StringComparer.OrdinalIgnoreCase.GetHashCode(o ?? "");

    public override bool Equals(object? obj) => obj is Struct1 other && string.Equals(o, other.o, StringComparison.OrdinalIgnoreCase);
}

Struct1 a1 = new() { o = "ABC" }, b1 = new() { o = "abc" };
Struct2 a2 = new() { field = a1 }, b2 = new() { field = b1 };
Console.WriteLine(a1.Equals(b1)); // true
Console.WriteLine(a1.GetHashCode() == b1.GetHashCode()); // true
Console.WriteLine(a2.Equals(b2)); // true
Console.WriteLine(a2.GetHashCode() == b2.GetHashCode()); // false

This can be fixed in follow-up.

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

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

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.

Comment on lines 311 to 312
obj1.o = null;
obj1.value.value = 2;
Copy link
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?

@@ -299,6 +299,21 @@ public static void StructContainsPointerCompareTest()
Assert.True(obj1.Equals(obj2));
Assert.Equal(obj1.GetHashCode(), obj2.GetHashCode());
}

[Fact]
public static void StructContainsPointerNestedCompareTest()
Copy link
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.

Copy link
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.

@jkotas
Copy link
Member

jkotas commented Jan 31, 2024

The new test is failing on native AOT:

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

@jkotas
Copy link
Member

jkotas commented Jan 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.

@AaronRobinsonMSFT AaronRobinsonMSFT added the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 6, 2024
@huoyaoyuan
Copy link
Member Author

huoyaoyuan commented Feb 20, 2024

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 meeting null also makes the implementation more complicated. I'd like to keep the behavior as-is and limit the test to run on coreclr only.

@huoyaoyuan
Copy link
Member Author

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

@jkotas
Copy link
Member

jkotas commented Feb 20, 2024

Should we change the behavior instead?

I do not think we should be changing the behavior. It can result into arbitrarily large perf regression of hashtable lookups.

@huoyaoyuan
Copy link
Member Author

Is there any other change required? I don't think it's necessary to test such implementation detail.

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.

Thank you!

@jkotas jkotas merged commit 8cc7586 into dotnet:main Feb 21, 2024
150 of 153 checks passed
@jkotas jkotas removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 21, 2024
@huoyaoyuan huoyaoyuan deleted the cancomparebits branch February 21, 2024 04:45
davidwrighton added a commit to davidwrighton/runtime that referenced this pull request Mar 6, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-coreclr 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.

3 participants