Replace multi-loaderallocator hash implementation in MethodDescBackpatchInfo #22285
Conversation
- Hashtable implementation for runtime use - Implementation written in C++ - Data storage in managed heap memory - Based on SHash design, but using managed memory CrossLoaderAllocatorHash - Hash for c++ Pointer to C++ pointer where the lifetimes are controlled by different loader allocators - Support for add/remove/visit all entries of 1 key/visit all entries/ remove all entries of 1 key - Supports holding data which is unmanaged, but data items themselves can be of any size (key/value are templated types)
|
@davmason I've put this up for review as a WIP to get cross platform testing. I believe its functionally correct, but as its a complex mass of templates currently only tested on msvc, I expect to see new and interesting failures on clang. Let me know what you think of the available api surface, and if it will work for your scenario. |
|
@davidwrighton I plan to take a look later today |
|
@janvorli I think I'd like you to take a look at this. This is a data structure that allows previously difficult cross loader allocator references to be kept. |
|
Looks good from the API surface area point of view. I don't see any issues |
- Adjust the Crst so that it can safely be used around code which allocates - Required moving its use out from within the EESuspend logic used in reji
…avidwrighton/coreclr into methodbackpatchhashreplacement
There was a problem hiding this comment.
Overall it looks good to me, my comments are mostly formal. I would also like to suggest to change the naming convention for members from _xxxx to m_xxxx for the sake of consistency with the rest of the runtime. That is the convention used in the whole runtime except for very few places.
- Adjust MethodDescBackpatchInfoTracker Crst to be correctly levelled
|
@dotnet-bot test this please |
- Use m_ instead of _ - Use m_gcHeapHash instead of _gcHeap to describe hashtable held in GC memory
davidwrighton
changed the title
There was a problem hiding this comment.
If you haven't already, could you please check to make sure that vtable slots get backpatched as expected with tiering enabled, something like for the non-generic cases in https://github.com/dotnet/coreclr/blob/master/tests/src/baseservices/TieredCompilation/TieredVtableMethodTests.cs including the cross-loader-allocator ones?
|
From my understanding, the dependent handle in If correct so far, from that point, does the finalizer of Wondering if there could be an issue where the |
No, this isn't an issue. The dependent handle loses its reference to the secondary when the secondary is actually unreachable (even from within a finalizer). The finalizer actually needs to be delayed until there cannot be an attempt to use the dependent handle to find an answer, hence the use of a GC handle with registered cleanup instead of a LoaderAllocator handle. |
| @@ -718,6 +719,8 @@ void TieredCompilationManager::ActivateCodeVersion(NativeCodeVersion nativeCodeV | |||
| // code version will activate then. | |||
| ILCodeVersion ilParent; | |||
| HRESULT hr = S_OK; | |||
| MethodDescBackpatchInfoTracker::ConditionalLockHolder lockHolder; | |||
There was a problem hiding this comment.
This could also pass in pMethod->MayHaveEntryPointSlotsToBackpatch() as a parameter
|
I see, makes sense |
|
@dotnet-bot test Windows_NT x64 Checked CoreFX Tests |
|
@dotnet-bot test Windows_NT x64 Checked Innerloop Build and Test |
…tchInfo (dotnet/coreclr#22285) * GCHeapHash - Hashtable implementation for runtime use - Implementation written in C++ - Data storage in managed heap memory - Based on SHash design, but using managed memory CrossLoaderAllocatorHash - Hash for c++ Pointer to C++ pointer where the lifetimes are controlled by different loader allocators - Support for add/remove/visit all entries of 1 key/visit all entries/ remove all entries of 1 key - Supports holding data which is unmanaged, but data items themselves can be of any size (key/value are templated types) * Swap MethodDescBackpatchInfo to use the CrossLoaderAllocatorHash * The MethodDescBackpatchCrst needs to be around an allocation - Adjust the Crst so that it can safely be used around code which allocates - Required moving its use out from within the EESuspend logic used in rejit Commit migrated from dotnet/coreclr@423d2a3
The multi-loaderallocator table for MethodDescs as used by MethodDescBackpatchInfo is both complex on shutdown (which is very difficult to test), somewhat memory inefficient, and not generalized for use for other similar needs. This change builds a new hashtable that utilizes the GC to handle lifetime, but is still useable from the portions of our codebase which need to use this data structure (and thus can't be written in managed code) See the comment in CrossLoaderAllocatorHash.h for details on how it works. It is expected that this will be used by this, by work adding support to the typesystem to track derived types, and by upcoming profiler api work (which has driven much of the complex size saving work, as it is expected that data structure will be large).