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

Include hashes of containing types when computing hash in EEClassHashTable #61652

Closed
wants to merge 4 commits into from

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Nov 16, 2021

Prior to this change all nested types with the same name would be placed into the same hash bucket, regardless of their enclosing types. That ensures collisions for common type names like Enumerator or <>c .

The approach in this change is similar to what we do in R2R hash - the hash function will now include hashes of enclosing types if such exist. The main difference from R2R approach is that we do this only for case-sensitive flavor of the table.
Case-insensitive table may be created as needed from case-sensitive prototype. Case-insensitive table by construction shares the encloser items with the prototype table and thus cannot use encloser hashes, since they would not be in the canonical casing.
That basically means the case-insensitive use stays on the same plan as before. It appears to be a relatively rare scenario to support some reflection features.


After measuring, the effects of the change appear to be fairly minor. It does reduce some collisions, but it is not a lot to start with and in case-insensitive case the collisions are still there, so we still have outliers.
I am not sure the result justifies the added complexity.

@dotnet-issue-labeler
Copy link

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.

@VSadov VSadov changed the title Consider containing types when computing hash in EEClassHashTable Include hashes of containing types when computing hash in EEClassHashTable Nov 16, 2021
@VSadov VSadov marked this pull request as ready for review November 16, 2021 23:27
@VSadov VSadov marked this pull request as draft November 17, 2021 00:27
@VSadov VSadov closed this Nov 17, 2021
@VSadov
Copy link
Member Author

VSadov commented Nov 17, 2021

@jkotas - FYI, I have implemented this change to reduce hash collisions in EEClassHashTable, but decided that the results are not worth the added complexity.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant