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.

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

Jump to bottom

Fix Type.GetHashCode for RuntimeTypes w/o MethodTable #97195

Merged
merged 1 commit into from
Jan 22, 2024
Merged

Fix Type.GetHashCode for RuntimeTypes w/o MethodTable #97195

merged 1 commit into from
Jan 22, 2024

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Jan 19, 2024

No description provided.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 19, 2024
@ghost ghost assigned jkotas Jan 19, 2024
@jkotas
Copy link
Member Author

jkotas commented Jan 19, 2024

/azp run runtime-nativeaot-outerloop

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Jan 19, 2024
Closed
@jkotas
Copy link
Member Author

jkotas commented Jan 19, 2024

/azp run runtime-nativeaot-outerloop

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Jan 19, 2024
Open
Closed
@jkotas
Fix Type.GetHashCode for RuntimeTypes w/o MethodTable
be47460 
The hashcode for these was always 0 before this change.
@jkotas jkotas added area-NativeAOT-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jan 19, 2024
@ghost
Copy link

ghost commented Jan 19, 2024

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

Issue Details

null

Author: jkotas
Assignees: jkotas
Labels:

area-NativeAOT-coreclr, needs-area-label
Milestone: -

@jkotas jkotas marked this pull request as ready for review January 19, 2024 17:45
@jkotas
Copy link
Member Author

jkotas commented Jan 19, 2024

@MichalStrehovsky PTLA. It is fixing a minor bug introduced by the RuntimeType refactoring.

This was referenced Jan 19, 2024
Open
Closed
MichalStrehovsky
MichalStrehovsky approved these changes Jan 22, 2024
View reviewed changes
Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

Thanks!

Do we want to add the test you had here before the force push?

@jkotas
Copy link
Member Author

jkotas commented Jan 22, 2024

Do we want to add the test you had here before the force push?

My original assumption was that this bug is doing to lead to crash that is not actually the case. It is not functionality bug; it is a perf bug.

The test for this bug would have to generate number of types and verify that their hashcode distibution hits some minimum threshold. We do not have tests like for GetHashCode implementations, so building one for RuntimeType did not seem to be worth it.

@jkotas jkotas merged commit 41494ba into dotnet:main Jan 22, 2024
107 of 110 checks passed
@jkotas jkotas deleted the fix-gethashcode branch January 23, 2024 02:02
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
@jkotas @tmds
Fix Type.GetHashCode for RuntimeTypes w/o MethodTable (dotnet#97195)
300f0f0 
The hashcode for these was always 0 before this change.
@github-actions github-actions bot locked and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MichalStrehovsky MichalStrehovsky MichalStrehovsky approved these changes

Assignees

@jkotas jkotas

Labels
area-NativeAOT-coreclr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jkotas @MichalStrehovsky