Simplify methodtable #96466
Simplify methodtable #96466
Conversation
…e and the GetLoaderModule path somewhat more complex - GetModule is more commonly used - GetLoaderModule is still pretty cheap. It just has 1 extra memory indirection. - This change adds 1 pointer to the size of all allocated types, which is a bit unfortunate, but it isn't that expensive. Adjust LoaderHeap handling to pass around LoaderAllocators instead, this is useful for passing through a LoaderAllocator to the CreateMinimalMethodTable code.
- This reduces the complexity of looking it up (it is now straightforward code) - Also adjust WFLAGS2 so that there are only 8 bits used in it
…ead of overflow member
… into following the MethodTableWriteableData
…bleData at a fixed offset - Access to this thing will no longer require computing the number of virtuals - By being located at a fixed offset, it becomes simpler to debug manually should that be necessary - Performance of this should be nicely maintained
…rol multipurpose slot allocation behavior anymore
- Make DAC a bit more robust against failure - Fix issue where now that the DispatchMap is being constructed before the MethodTable, we cannot assert if a MethodDesc is virtual
ghost
assigned 
- Array MethodTables now return FALSE for HasSameTypeDefAs - This required a tweak in CEEInfo::embedGenericHandle to make it behave consistently with previous behavior
…me into simplify_methodtable
| }; | ||
|
|
||
|
|
||
| PTR_Module m_pLoaderModule; |
There was a problem hiding this comment.
Is there a reason for m_pLoaderModule to be here and not in the MethodTable?
There was a problem hiding this comment.
No strong reason, I was trying to avoid growing the MethodTable structure, as growth has a (very) small impact on v-table slots due to the limited range of a mov instruction on x64, but its not a critical issue.
Effectively, there is a small size optimization to be had in generated code for virtual function access where if the offset of the vtable chunk index is less than 256 bytes from the start of the MethodTable, we can use a 3 byte instruction instead of a 6 byte instruction. It's not significant for throughput perf, but historically I remember seeing some not completely negligible impact on code size for ngen images when adjusting the size of MethodTable. However, of course, ngen is no longer a thing, so perhaps I shouldn't worry about that, and looking at the data, I see that it would only matter for virtual table slots 25-32 if we moved two pointer sized values down from MethodTableWriteableData to MethodTable, which probably isn't significant at all. (Possibly my memories are from the really before times, when we didn't have vtable chunks, and this change would have effected vtable slots 7 and 8, which are fairly common in code bases)
From what I can see the m_pLoaderModule's only real perf sensitive use is as part of statics lookup, and I'm planning to make that simpler in a followon checkin, which is why I didn't keep it at the MethodTable level, but you are correct, it probably would be fine as a field of MethodTable (along with the m_hExposedClassObject field which also has most of the same properties) I'd love to hear if anyone else has an opinion here.
There was a problem hiding this comment.
I think that the state in this PR is fine. We can fine-tune it in follow up changes.
src/coreclr/vm/methodtable.h
Outdated
| { | ||
| return dac_cast<PTR_GenericsStaticsInfo>(dac_cast<TADDR>(pWriteableData) - sizeof(GenericsStaticsInfo)); | ||
| } | ||
| }; // struct MethodTableWriteableData |
There was a problem hiding this comment.
Should MethodTableWriteableData be renamed to something that better captures its current purpose? Like MethodTableAuxiliary data?
There was a problem hiding this comment.
Ah, great, that's such a nice name. I didn't have a good one in mind, so I wasn't changing it, but MethodTableAuxiliaryData is a distinct improvement, especially since the writeable concept doesn't make any sense anymore.
| // This also disables IBC logging until the type is sufficiently initialized so | ||
| // it needs to be done early |
There was a problem hiding this comment.
Is this comment still relevant?
There was a problem hiding this comment.
No. This comment could be deleted. I'll do that in a followon PR.
|
|
||
| size_t prependedAllocationSpace = 0; | ||
|
|
||
| prependedAllocationSpace = nonVirtualSlots * sizeof(TADDR); |
There was a problem hiding this comment.
Just initialize prependedAllocationSpace to this instead of 0?
|
The only other thing I'd ask is if there is anything that needs to be updated in the BoTR. I did a quick check and nothing jumped out, but it would be helpful if you could do a quick check. |
|
@AaronRobinsonMSFT I've taken a quick look at the BoTR. All these details have apparently fallen under the category of trivial details not mentioned in the docs. |
In particular, simplify access to fields of the MethodTable structure Historically, this structure had a great many optional fields in it with several mechanisms for identifying them. As we've been working on CoreCLR, we've eliminated many of these excess fields, to the point where we likely have a final list of all expandable fields associated with the MethodTable, and we have space to make access to fields somewhat simpler. Notably, this is an effort to reduce the number of memory reads and calculations necessary to find the various parts of a MethodTable. With this change, I believe that all data directly associated the MethodTable struct can be accessed with no more than 2 pointer chasing memory accesses and possibly reading 1 set of flags. This should simplify much of the codegen around this structure, although performance is likely unaffected, as most of the slow paths were very well optimized by native PGO technologies.
In particular, simplify access to fields of the
MethodTablestructureHistorically, this structure had a great many optional fields in it with several mechanisms for identifying them. As we've been working on CoreCLR, we've eliminated many of these excess fields, to the point where we likely have a final list of all expandable fields associated with the
MethodTable, and we have space to make access to fields somewhat simpler.Notably, this is an effort to reduce the number of memory reads and calculations necessary to find the various parts of a
MethodTable. With this change, I believe that all data directly associated the MethodTable struct can be accessed with no more than 2 pointer chasing memory accesses and possibly reading 1 set of flags. This should simplify much of the codegen around this structure, although performance is likely unaffected, as most of the slow paths were very well optimized by native PGO technologies.