Improve the hashtable for EEClassHash#94825
Conversation
…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
added
the
|
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) |
davidwrighton
changed the title
AaronRobinsonMSFT
left a comment
AaronRobinsonMSFT
left a comment
There was a problem hiding this comment.
A few nits, but overall looks like a win.
|
|
||
| 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.
| // 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
| 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
left a comment
AaronRobinsonMSFT
left a comment
There was a problem hiding this comment.
A few nits, but overall looks like a win.
4 participants
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.