-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Cached interface dispatch for coreclr #111771
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2b2ca52
4892674
d69528f
5f1f2b5
976bf83
652930c
39a2574
4c0865c
645c487
921631a
73b0b26
2cdd955
edad834
df393d9
cce3bcb
4bbcdaa
c320e1d
361588a
24e78b2
5b0e5ac
fa7826a
f1c2c65
36c9cc0
9377342
a3a4ff1
18b3f13
80ae02b
081fac5
a0b9d2a
dcbe17c
6e5a394
48b5009
ad64a55
fa72602
08ac0a1
006537d
2ff4d2a
6cb2b78
70dacc0
3f44b9b
bea72d0
98a186e
2b744f8
becc4ce
5bd7040
0ac5629
301150e
18fd107
1c7e310
0380766
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3620,14 +3620,19 @@ ClrDataAccess::TraverseVirtCallStubHeap(CLRDATA_ADDRESS pAppDomain, VCSHeapType | |
break; | ||
|
||
case CacheEntryHeap: | ||
#ifdef FEATURE_VIRTUAL_STUB_DISPATCH | ||
// The existence of the CacheEntryHeap is part of the SOS api surface, but currently | ||
// when FEATURE_VIRTUAL_STUB_DISPATCH is not defined, the CacheEntryHeap is not created | ||
// so its commented out in that situation, but is not considered to be a E_INVALIDARG. | ||
pLoaderHeap = pVcsMgr->cache_entry_heap; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we get a small comment here on why the CacheEntryHeap is not used in the caching logic? I'm ignorant of this area and it feels odd that the VirtualCallStubManager is used, but ignored in some cases. Perhaps an assert or some other breadcrumb to indicate why this isn't appropriate. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could use it, and possibly should, but as I noted in the PR description, I intentionally disabled this feature since it was more work to enable, and this change is already too big for easy development. If we decide that this sort of caching is advantageous, it would be appropriate in a separate PR to enable it for the cached interface dispatch scenario. |
||
#endif // FEATURE_VIRTUAL_STUB_DISPATCH | ||
break; | ||
|
||
default: | ||
hr = E_INVALIDARG; | ||
} | ||
|
||
if (SUCCEEDED(hr)) | ||
if (SUCCEEDED(hr) && (pLoaderHeap != NULL)) | ||
{ | ||
hr = TraverseLoaderHeapBlock(pLoaderHeap->m_pFirstBlock, pFunc); | ||
} | ||
|
@@ -3670,7 +3675,9 @@ static const char *LoaderAllocatorLoaderHeapNames[] = | |
"FixupPrecodeHeap", | ||
"NewStubPrecodeHeap", | ||
"IndcellHeap", | ||
#ifdef FEATURE_VIRTUAL_STUB_DISPATCH | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This adds to the confusion from above considering |
||
"CacheEntryHeap", | ||
#endif // FEATURE_VIRTUAL_STUB_DISPATCH | ||
}; | ||
|
||
|
||
|
@@ -3714,7 +3721,9 @@ HRESULT ClrDataAccess::GetLoaderAllocatorHeaps(CLRDATA_ADDRESS loaderAllocatorAd | |
else | ||
{ | ||
pLoaderHeaps[i++] = HOST_CDADDR(pVcsMgr->indcell_heap); | ||
#ifdef FEATURE_VIRTUAL_STUB_DISPATCH | ||
pLoaderHeaps[i++] = HOST_CDADDR(pVcsMgr->cache_entry_heap); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it safe to skip initializing this slot? The other branch explicitly zeroes out unused slots but we're not doing that here |
||
#endif // FEATURE_VIRTUAL_STUB_DISPATCH | ||
} | ||
|
||
// All of the above are "LoaderHeap" and not the ExplicitControl version. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -529,3 +529,6 @@ End | |
Crst PerfMap | ||
AcquiredAfter CodeVersioning AssemblyList | ||
End | ||
|
||
Crst InterfaceDispatchGlobalLists | ||
End |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8408,16 +8408,8 @@ class Compiler | |
reg = REG_EAX; | ||
regMask = RBM_EAX; | ||
#elif defined(TARGET_AMD64) | ||
if (isNativeAOT) | ||
{ | ||
reg = REG_R10; | ||
regMask = RBM_R10; | ||
} | ||
else | ||
{ | ||
reg = REG_R11; | ||
regMask = RBM_R11; | ||
} | ||
reg = REG_R11; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AAGGHH.. reading this has sent me down the rabbit hole of sadness which has told me that I need to move both CoreCLR and NativeAOT to use r10, and not to r11 as that is the only way to keep CFG from exploding sadly. (Or I can keep the difference between CoreCLR and NativeAOT). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea of getting these unified as you may guess. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or rather... this won't break stuff, but the codegen for CFG is required to be a bit non-ideal in native aot..OTOH, it appears with some quick testing that this doesn't actually effect the codegen in any case, since our CFG generation logic isn't what I'd call great right now, so I'd like to keep moving everything to r11. If we want to use r10 we can swap back all of the amd64 stuff at once. |
||
regMask = RBM_R11; | ||
#elif defined(TARGET_ARM) | ||
if (isNativeAOT) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,3 +76,16 @@ class VMToOSInterface | |
// true if it succeeded, false if it failed | ||
static bool ReleaseRWMapping(void* pStart, size_t size); | ||
}; | ||
|
||
#if defined(HOST_64BIT) && defined(FEATURE_CACHED_INTERFACE_DISPATCH) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice if we added the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately, this is a case where we want to have a shared set of PAL apis between CoreCLR and NativeAOT, and NativeAOT hasn't moved towards the VmToOSInterface idea (And this change is already WAY too big to add that to it.) I'd welcome some rationalization after this is all merged. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see we're adding a HOST_64BIT guard here but when I look at the CMakeLists changes it doesn't seem like we're necessarily doing the work to enable 128-bit cmpxchg for all 64-bit targets. Is this going to be fine on all the 64-bit targets we support or do we need to guard it with more specific conditionals? I know we have some support for more esoteric architectures than arm64 and amd64, and I'm not sure whether those are 64-bit hosts. 64-bit WASM doesn't have 16-byte cmpxchg, but it's probably safe to ignore that one since most WASM applications are using a 32-bit address space (it's much faster) where an 8-byte cmpxchg is good enough There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was discussed above #111771 (comment), currently it's only enabled for amd64 and arm64 for non-nativeaot parts. Also, amd64 has |
||
EXTERN_C uint8_t _InterlockedCompareExchange128(int64_t volatile *, int64_t, int64_t, int64_t *); | ||
|
||
#if defined(HOST_WINDOWS) | ||
#pragma intrinsic(_InterlockedCompareExchange128) | ||
#endif | ||
|
||
FORCEINLINE uint8_t PalInterlockedCompareExchange128(_Inout_ int64_t volatile *pDst, int64_t iValueHigh, int64_t iValueLow, int64_t *pComparandAndResult) | ||
{ | ||
return _InterlockedCompareExchange128(pDst, iValueHigh, iValueLow, pComparandAndResult); | ||
} | ||
#endif // defined(HOST_64BIT) && defined(FEATURE_CACHED_INTERFACE_DISPATCH) |
Uh oh!
There was an error while loading. Please reload this page.