-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Cache Sytem.RuntimeMethodInfoStub instances created in the VM in the MethodDesc's owning LoaderAllocator. #26492
Conversation
…MethodDesc's owning LoaderAllocator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see thread contention issues, and incorrect use of HashMap. Please address.
} | ||
CONTRACTL_END; | ||
|
||
CrstHolder holder(&m_stubMethodInfoCacheCrst); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting a Crst here adds possible thread contention to an api which was previously effectively lock free.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead, I would do the lookup without the lock, then take the lock, lookup again, and then add. Both the SHash infrastructure and HashMap are supposed to be able to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SHash does not support lock-free reads.
|
||
// Used for synchronizing access to m_stubMethodInfoCache. | ||
CrstExplicitInit m_stubMethodInfoCacheCrst; | ||
HashMap m_stubMethodInfoCache; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use HashMap directly for holding pointers. Per the comments on the PtrHashMap implementation, you should never put use a pointer as a key in a HashMap, you should always use a PtrHashMap. Of course, you can't safely store a LOADERHANDLE in a PtrHashMap, as it requires the low bit to be null on values. Instead I recommend you use a MapSHash.
Why are the allocations of |
In #26340, the efficient way to support marshaling arrays of non-blittable structures is to cache the MethodDesc of the struct stub. To do so, I was doing a This was my first-draft attempt at caching the allocation so we wouldn't allocate every call. Additionally, it seemed odd to me initially that the |
|
What are you going to do with this token? You cannot really do anything efficient with it. I assume that you want to call the function eventually - would it be better to do |
I'm getting the InternalToken to pass the MethodDesc pointer back down to cache in the structure for the marshaler (since it needs to be invoked from the array marshalling which is still in native code). Is there a way I could call from native back into managed within the runtime via a function pointer to a managed function? I've been using MethodDescCallSite so far. |
Take a look at |
I'll take a look at it. Thanks! |
That worked perfectly! Thanks for the suggestion! |
Instead of allocating a new
System.RuntimeMethodInfoStub
every timeMethodDesc::GetStubMethodInfo
is called, lazily create the stub and cache it in the MethodDesc's LoaderAllocator.This change was extracted out of #26340.