Change CrossLoaderAllocatorHash to not use the GC

[kouvel]

GCHeapHash:

• Replaced GCHeapHash usage with SHash
• Added s_supports_autoremove to SHash to simulate GCHeapHash's ability to remove elements while iterating them. When enabled, SHash calls ShouldDelete() on elements it walks during various operations and removes them if requested
• Added an optional on-remove cleanup action for elements in SHash to ease cleanup of heap-allocated elements

CrossLoaderAllocatorHash:

• Added formal native types for internal data structures and hash table elements. The layout of the internal data structures is mostly the same as before.
• Trackers are ref-counted since they may be inserted into multiple hash tables, and get deleted once their ref count reaches zero
• Translated the current implementation. Used holders where necessary to handle OOMs gracefully.

Miscellaneous:

• Removed cooperative GC mode usage in the relevant places in slot backpatching and inline tracking, and removed usage of the forbid-suspend-for-debugger region in those places
• When deleting call counters, reordered such that the runtime is suspended before the slot backpatching lock is acquired, to prevent the suspension code from suspending for the debugger while the lock is held and leading to FuncEval deadlocks

Testing:

• Memory usage is roughly the same as before, if anything it seems to be a bit lower
• Perf is similar to before, no significant change
• Ran a slot backpatching test including collectible assemblies in stress mode to look for issues
• Verified a couple of the debugger-attach deadlock scenarios with induced timings

Fix for #58471 and #66759 in main

[kouvel]

For crossloaderallocatorhash.h/inl here are better-aligned diffs for easier review (download and open in browser)

[noahfalk]

Hey @kouvel I'm still working my way through this code but here are some comments so far. Most of what I've looked at was good with minor questions/suggestions but the change in RareDisablePreemptiveGC did look worrisome. I'll continue reviewing tomorrow.

[noahfalk]

Isn't Add inherently a mutating operation? I don't understand the const keyword here.

[kouvel]

This private overload was previously static, I changed a couple of static functions to be instance functions with const because they needed to call RemoveElement, which is an instance function, to support autoremove. In this case it would make sense to make it non-const in its implementation, fixed.

[noahfalk]

In the destructor you preserved the previous behavior for Hashtables that had a DestructPerEntryCleanupAction but here the behavior changed. Is there a reason they are handled differently? If we revert to a single cleanup action then I expect this behavior would return to what it was before. If we stick with two separate ones it raises the question whether this change in behavior broke any code that previously expected to run the destruct action during RemoveAll() and now that doesn't happen.

[kouvel]

Technically it's not accurate to call the on-destruct cleanup action when the hash table is not being destructed in RemoveAll. s_DestructPerEntryCleanupAction isn't used with RemoveAll currently or with any of the other functions that require s_supports_remove. s_DestructPerEntryCleanupAction may make sense in cases where the user of a hash table may want to manage the lifetime of elements separately until the hash table itself is destructed, so for RemoveAll, similarly to Remove, the expectation would be that the user manages the lifetime of removed elements. There is a case currently where s_DestructPerEntryCleanupAction is being used with AddOrReplace with the expectation that the replaced element does not trigger the cleanup action, so s_RemovePerEntryCleanupAction would not be appropriate there:

runtime/src/coreclr/binder/applicationcontext.cpp

Lines 114 to 166 in 7d562f9

	const SimpleNameToFileNameMapEntry *pExistingEntry = m_pTrustedPlatformAssemblyMap->LookupPtr(simpleName.GetUnicode());
	if (pExistingEntry != nullptr)
	{
	//
	// We want to store only the first entry matching a simple name we encounter.
	// The exception is if we first store an IL reference and later in the string
	// we encounter a native image. Since we don't touch IL in the presence of
	// native images, we replace the IL entry with the NI.
	//
	if ((pExistingEntry->m_wszILFileName != nullptr && !isNativeImage) ||
	(pExistingEntry->m_wszNIFileName != nullptr && isNativeImage))
	{
	continue;
	}
	}
	LPWSTR wszSimpleName = nullptr;
	if (pExistingEntry == nullptr)
	{
	wszSimpleName = new WCHAR[simpleName.GetCount() + 1];
	if (wszSimpleName == nullptr)
	{
	GO_WITH_HRESULT(E_OUTOFMEMORY);
	}
	wcscpy_s(wszSimpleName, simpleName.GetCount() + 1, simpleName.GetUnicode());
	}
	else
	{
	wszSimpleName = pExistingEntry->m_wszSimpleName;
	}
	LPWSTR wszFileName = new WCHAR[fileName.GetCount() + 1];
	if (wszFileName == nullptr)
	{
	GO_WITH_HRESULT(E_OUTOFMEMORY);
	}
	wcscpy_s(wszFileName, fileName.GetCount() + 1, fileName.GetUnicode());
	SimpleNameToFileNameMapEntry mapEntry;
	mapEntry.m_wszSimpleName = wszSimpleName;
	if (isNativeImage)
	{
	mapEntry.m_wszNIFileName = wszFileName;
	mapEntry.m_wszILFileName = pExistingEntry == nullptr ? nullptr : pExistingEntry->m_wszILFileName;
	}
	else
	{
	mapEntry.m_wszILFileName = wszFileName;
	mapEntry.m_wszNIFileName = pExistingEntry == nullptr ? nullptr : pExistingEntry->m_wszNIFileName;
	}
	m_pTrustedPlatformAssemblyMap->AddOrReplace(mapEntry);

[noahfalk]

s_DestructPerEntryCleanupAction isn't used with RemoveAll currently or with any of the other functions that require s_supports_remove

Do you mean that you have audited all usage of shash RemoveAll() in the runtime and determined that there is no overlap between RemoveAll() users and s_DestructPerEntryCleanupAction users?

[kouvel]

I believe so, I added a static assert on all of those paths that s_DestructPerEntryCleanupAction can't be true and it compiled, so if there were such a call it should have resulted in a compile error.

[kouvel]

Also looked at the few cases where s_DestructPerEntryCleanupAction is set to true and didn't see anything like that. I think I had added the s_DestructPerEntryCleanupAction check in RemoveAll along with the slot backpatching change.

[davidwrighton]

I should learn to read the previous comments better. .... You're right. That does depend on the current OnDestruct behavior. Well. This is why its a C++ template.

[noahfalk]

Suggested change

	if (TRAITS::s_supports_autoremove && TRAITS::ShouldDelete(current))
	if ( (TRAITS::s_supports_autoremove && TRAITS::ShouldDelete(current)) ||
	(TRAITS::Equals(key, TRAITS::GetKey(current))) )
	{
	RemoveElement(table, tableSize, &current);
	}

I think the code in the else block has the identical effect as calling RemoveElement or did I miss a subtle difference?

[noahfalk]

I would add a comment like this. I want to make it clear that this code already appears to be unsafe.

Suggested change

	// To be used when the thread may enter cooperative GC mode while holding the lock. The thread enters a
	// BUGBUG: This holder creates a region of code where debugger suspension is
	// prevented from completing. EE suspension may not be able to complete until
	// debugger suspend finishes successfully (see "retry_for_debugger" in
	// ThreadSuspend::SuspendEE). This means that any code inside this holder which
	// waits for an EE suspension to complete directly or indirectly may deadlock.
	// Although we haven't caught a live example of this issue under a debugger we
	// have customer reports of issues which fit this description and code review shows
	// that we have code paths inside this holder which invoke GC_TRIGGERS methods.
	// Any further use of this holder should exercise great caution not to worsen the
	// deadlock issue that is likely already present. Ideally we should be eliminating the
	// forbid debugger suspend component of this holder or fixing the code that runs
	// inside it to avoid any potential to block on EESuspend.

[noahfalk]

I think we need to modify this comment to say the code inside the holder can't block either directly or indirectly on EESuspend. GC allocation is one way that dependency could happen but there are other ways, such as a thread which does a preemptive->coop mode transition. Given that the stated purpose of the holder is to allow code to do a GC mode switch but also doing that transition risks deadlock we should probably comment up-front that there is no known safe+productive usage of this holder.

[noahfalk]

Suggested change

	// would either it cleared, or that the dependent object remains valid under those locks. In particular, any locks used to
	// would either observe it cleared, or that the dependent object remains valid under those locks. In particular, any locks used to

I think that is what you meant?

[noahfalk]

If I am understanding this correctly this looks dangerous. The invariant of coop-mode is that threads can not run in this mode while the EE is suspended. This logic appears to say that invariant no longer applies to threads in a debugger suspend region. This means those threads could do things such as GC allocations, manipulate OBJECTREFs, or run managed code while an in-progress GC believes that all threads interacting with managed state are suspended.
If threads inside a debugger forbid suspend region are needlessly entering coop mode I think we should try to change their code path so that they stop entering coop mode. And if it is a requirement that they enter coop mode that presumably means they are doing something that is unsafe to be running concurrently with the GC and correctness requires them to block here.

[kouvel]

You're right, this is not sufficient. This thread would have to be considered "not yet suspended", which is done currently for debugger suspension but not for GC suspension. I might just revert this part of the change for the forbid regions from this PR and look into it a bit closer separately.

[noahfalk]

Rather than having two different cleanup operations could we have a single cleanup operation with a boolean flag that says whether destruct also runs in the Remove() function? Having two of them makes it harder to know which one will run in which circumstances. I didn't notice that we have any scenario that depends on having both of the cleanup actions set simultaneously so it feels like this supports more options than our code needs.

[kouvel]

I guess either way it would be two flags, I think these names make it a bit more clear about when they run, that is the OnDestruct cleanup action only runs when the hash table is destructed with the expectation that the user manages the lifetime of elements until then, and the OnRemove cleanup action runs any time an element is removed, including when the hash table is destructed, enabling the cleanup action for full lifetime management of elements. I don't think it makes sense to use both at the same time, for now I've added a static assert ensuring that both can't be enabled.

[davidwrighton]

Looking at the existing uses of OnDestruct, they look like they would work just fine with the newer OnRemove model, and in fact are not really designed for the OnDestruct scenario. I'm not convinced we should keep OnDestruct at all.

[noahfalk]

Btw @kouvel, what did you use to produce those custom html diffs? They are very useful.

[noahfalk]

OK I finished reviewing and the remainder of the code looked good. All the interesting comments were the ones I left last time. Thanks Kount!

[noahfalk]

This works though it seems a little awkward that each new CrossLAHash has to insert its lock here and all lock owners will need to work out their proper ordering relative to each other even if their locks would never otherwise be taken at the same time. Its not a big deal with only two users but if we start accruing more we might want to switch to a different pattern. One possibility would be adding a method to CrossLAHash traits which runs a lamda synchronized against all other accesses to that hash (presumably by doing it under the lock they would have added here). Then the DependentHandle could invoke that traits function passing a lamda that will delete the native object and clear the field. This lets each handle take the lock appropriate for that hash rather than taking all locks simultaneously.

In any case, I'm fine with this as-is, this was just thoughts for the future.

[kouvel]

Agreed it's not ideal. The lambda approach is interesting but it might have an issue with lock ordering. Currently m_dependentHandleToNativeObjectSetCrst is taken after the locks used for synchronizing access to the CrossLAHash instances. Since the handle cleanup code has to iterate the set of handles, it would have to first take m_dependentHandleToNativeObjectSetCrst and then the traits method would acquire its lock in the opposite order. Still thinking about ways to improve it, but decided to keep it simple for now.

[noahfalk]

Yeah I noticed that and handwaved a bit that we could make a copy of the list before iterating it, or perhaps enter the lock to set a flag indicating that the LoaderAllocator is cleaning up so no new handles could be added. Happy to leave it how it is for now.

[davidwrighton]

I agree its awkward, but I do not expect there to be many of these hashtables. They are quite expensive and complicated. I do not believe its worthwhile to invest in improving an api that works well enough, as it is.

[kouvel]

Btw @kouvel, what did you use to produce those custom html diffs? They are very useful.

I use Beyond Compare, it has a very useful feature to manually align things and produce diff reports.

[kouvel]

@noahfalk I have reverted changes relevant to forbid-suspend-for-debugger regions for this PR. I'll look into fixing that or removing it in a separate PR and will incorporate your other suggestions into that.

[kouvel]

Looks like the failure is #66571

[davidwrighton]

could you give these locals a different name that doesn't overlap with the uses above?

[davidwrighton]

[adamsitnik]

There is a nice perf improvement that most likely was caused by this change:

https://pvscmdupload.blob.core.windows.net/reports/allTestHistory%2frefs%2fheads%2fmain_x64_Windows%2010.0.18362%2fSystem.Tests.Perf_GC(Char).AllocateUninitializedArray(length%3a%2010000%2c%20pinned%3a%20True).html

[noahfalk]

I'm curious, how is the perf improvement mapped to the change? I am a little surprised this change would make a difference here but if it did that is a nice bonus : )

[adamsitnik]

@noahfalk each point on the chart provided by the Reporting System represents a dotnet/runtime commit. We can select two of them and get the diff (this magic needs to get documented). For this improvement the diff points to the following range of commits: 322ea09...dee2bb5

[kouvel]

I'm not sure, I didn't expect it would have any effect in steady state. Some GC memory usage would have been reduced, maybe there's a secondary effect.

[noahfalk]

In addition to our unexpected perf gains, big thanks to getting this sorted out @kouvel. I know it had been lurking there a long time and took a good bit of effort to rework : )