Skip to content

Convert Valuetype.FastEqualsCheck and GetHashCodeOfPtr to managed - #69723

Merged
jkotas merged 7 commits into
dotnet:mainfrom
huoyaoyuan:valuetype-managed
May 29, 2022
Merged

Convert Valuetype.FastEqualsCheck and GetHashCodeOfPtr to managed#69723
jkotas merged 7 commits into
dotnet:mainfrom
huoyaoyuan:valuetype-managed

Conversation

@huoyaoyuan

@huoyaoyuanhuoyaoyuan commented May 24, 2022

Copy link
Copy Markdown
Member

I'm not sure whether such conversions is desired. Opening a PR for comment.
Can't find corresponding benchmark in performance repo.

@ghost

ghost commented May 24, 2022

Copy link
Copy Markdown

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghostghost added the community-contribution Indicates that the PR has been added by a community member label May 24, 2022
jkotas
jkotas reviewed May 24, 2022
View reviewed changes
Comment threadsrc/coreclr/System.Private.CoreLib/src/System/ValueType.cs Outdated
MichalStrehovsky added a commit that referenced this pull request May 25, 2022
@MichalStrehovsky
Speed up ValueType.Equals
67d96fe
Inspired by #69723 `SpanHelpers` didn't exist when this part of NativeAOT was written (dotnet/corert#5436 (comment)).
@MichalStrehovskyMichalStrehovsky mentioned this pull request May 25, 2022
Merged
@huoyaoyuan

huoyaoyuan commented May 25, 2022

Copy link
Copy Markdown
MemberAuthor

Additionally, do we need AgressiveInlining here? Inlining looks probably beneficial.

MichalStrehovsky added a commit that referenced this pull request May 25, 2022
@MichalStrehovsky
Speed up ValueType.Equals (#69768)
0dba0ee
Inspired by #69723 `SpanHelpers` didn't exist when this part of NativeAOT was written (dotnet/corert#5436 (comment)).
GSPP
GSPP reviewed May 25, 2022
View reviewed changes
Comment threadsrc/coreclr/System.Private.CoreLib/src/System/ValueType.cs

internal static int GetHashCodeOfPtr(IntPtr ptr)
{
int hashCode = (int)ptr;

@GSPPGSPPMay 25, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is dropping the upper bits of a 64 bit pointer. Is this wise? All pointers offset by 4GB will result in the same hash code.

@huoyaoyuanhuoyaoyuanMay 26, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This method is for runtime handles which are pointers to unmanaged tables. I don't expect they would span across 4GB.

huoyaoyuan
huoyaoyuan commented May 27, 2022
View reviewed changes
Comment threadsrc/coreclr/vm/comutilnative.cpp Outdated
@ghost

ghost commented May 27, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

I'm not sure whether such conversions is desired. Opening a PR for comment.
Can't find corresponding benchmark in performance repo.

Author:huoyaoyuan
Assignees:-
Labels:

area-System.Runtime, community-contribution

Milestone:-

jkotas
jkotas reviewed May 27, 2022
View reviewed changes
Comment threadsrc/coreclr/System.Private.CoreLib/src/System/ValueType.cs
jkotas
jkotas reviewed May 28, 2022
View reviewed changes
Comment threadsrc/coreclr/vm/comutilnative.h Outdated
public:
static FCDECL1(FC_BOOL_RET, CanCompareBits, Object* obj);
static FCDECL2(FC_BOOL_RET, FastEqualsCheck, Object* obj1, Object* obj2);
static FCDECL1(UINT32, GetNumInstanceFieldBytes, MethodTable* mt);

@jkotasjkotasMay 28, 2022

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.

Nit: It would be nice to put it into a separate type. I expect we are going to have more MethodTable methods like this over time.

class MethodTableNative {
public:
static FCDECL1(UINT32, GetNumInstanceFieldBytes, MethodTable* mt);
}

jkotas
jkotas approved these changes May 28, 2022
View reviewed changes

@jkotasjkotas left a comment

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.

LGTM otherwise. Thank you!

@jkotas

jkotas commented May 28, 2022

Copy link
Copy Markdown
Member

Could you please resolve the conflict?

@jkotas
jkotas merged commit 1466e40 into dotnet:mainMay 29, 2022
@huoyaoyuan
huoyaoyuan deleted the valuetype-managed branch May 29, 2022 13:46
@ghostghost locked as resolved and limited conversation to collaborators Jun 28, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@EgorBoEgorBoEgorBo left review comments
@GrabYourPitchforksGrabYourPitchforksGrabYourPitchforks left review comments
@jkotasjkotasjkotas approved these changes
+1 more reviewer
@GSPPGSPPGSPP left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Runtimecommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@huoyaoyuan@jkotas@EgorBo@GrabYourPitchforks@GSPP@marek-safar