Improve the hashtable for EEClassHash #94825
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…class - Move namespace/name splitting to the Type.GetType code paths - Move exported type handling into the normal PopulateAvailableClass flow - Remove unnecessary work done to detect typedef name duplicates. We don't attempt to protect against invaild assemblies anymore - Unify path for insertion between ExportedType and TypeDef records, also unify the path for nested vs non-nested - Fix logic which implements inserts into the case insensitive table when dynamically adding entries to the ExportedType table (Previously it didn't work) - Update the ECMA 335 augments to capture the requirement that nested ExportedTypes must have a higher RID than the enclosing ExportedType - This requirement has actually always existed since .NET 1.0, but was never recorded
…is a linear scan of the entire typedef table, and we already have the right hash to make this cheap
- Also make the code more DAC correct (probably not completely correct, but this logic does not appear to actually be used within the DAC)
ghost
assigned 
|
Before checkin, I need to validate that we actually have some tests for the TypeRef that points at a TypeDef scenario. (The code change related to this has been remove from this PR and moved to #95297) |
jkotas
reviewed
Nov 16, 2023
davidwrighton
changed the title
hughbe
reviewed
Nov 28, 2023
AaronRobinsonMSFT
approved these changes
Dec 5, 2023
| }; | ||
|
|
||
| inline DWORD GetHash(PTR_EEClassHashEntry eeclassHashEntry) | ||
| { | ||
| // This is only safe, as the only allocator for the EEClassHashEntry type is the hash table |
There was a problem hiding this comment.
Suggested change
| // This is only safe, as the only allocator for the EEClassHashEntry type is the hash table | |
| // This is safe, as the only allocator for the EEClassHashEntry type is the hash table |
Right?
src/coreclr/vm/classhash.cpp
Outdated
| #endif | ||
|
|
||
| // cqb - If m_bCaseInsensitive is true for the hash table, the bytes in Key will be allocated | ||
| // cqb - If IsCaseInsensitiveTable() is true for the hash table, the bytes in Key will be allocated |
There was a problem hiding this comment.
What is cqb? I don't see that anywhere in here.
There was a problem hiding this comment.
It isn't there at all. This comment is confusing as it refers to a state in the code which hasn't existed in decades. I've tried to fix up the comment to make some amount of sense, but in general everything to do with the case insensitive table is nonsense.
src/coreclr/vm/clsload.cpp
Outdated
| @@ -3865,11 +3587,13 @@ VOID ClassLoader::AddExportedTypeDontHaveLock(Module *pManifestModule, | |||
| AddExportedTypeHaveLock( | |||
| pManifestModule, | |||
| cl, | |||
| NULL, | |||
There was a problem hiding this comment.
Remove NULL and use empty SArray<EEClassHashEntry_t *>?
There was a problem hiding this comment.
Sure. I did it for the other case.
AaronRobinsonMSFT
approved these changes
Dec 5, 2023
This was referenced Jan 2, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
EEClassHashTableimplementation failed to use a hash function which included enclosing types, and has a variety of other deficiencies where it does unnecessary work. Include the enclosing type in the hash function, and reduce the amount of code duplication in the hash table.In addition, I'm taking this opportunity to clean up the implementation substantially, creating a single path for insertion, and lookup.