Look into the possibility of removing the crst for AssemblyIterator during GC

[mjsabby]

DevDiv workitem: https://devdiv.visualstudio.com/defaultcollection/DevDiv/_workitems/edit/125447/

From @jkotas in an internal email thread:

Right now, the list is manipulated in preemptive mode (AppDomain::AddAssembly / RemoveAssembly has MODE_ANY contract), so I do not think we can just remove this lock during GC. However, if we can change the code to always manipulate the list in cooperative mode, the lock can be removed during GC.

We should opened a bug on it, so that somebody looks into it in more details.

-Jan

Callstack causing the performance problem

    |+ clr!CrstBase::Enter
    | + clr!CrstBase::CrstHolder::CrstHolder
    | |+ clr!AppDomain::AssemblyIterator::Next
    | ||+ clr!AppDomain::EnumStaticGCRefs
    | || + clr!SystemDomain::EnumAllStaticGCRefs
    | ||  + clr!CNameSpace::GcScanRoots
    | ||   + clr!SVR::gc_heap::background_mark_phase
    | ||   |+ clr!SVR::gc_heap::gc1
    | ||   | + clr!SVR::gc_heap::bgc_thread_function
    | ||   |  + clr!Thread::intermediateThreadProc
    | ||   |   + kernel32!BaseThreadInitThunk
    | ||   |    + ntdll!RtlUserThreadStart

cc @Maoni0 @PeterSolMS

[jkotas]

Fixing the lock should not be hard.

I am wondering why we are doing this while the world is stopped at all. All statics are rooted by GC handles as well, so this piece of logic in GCToEEInterface::GcScanRoots is just frontloading scanning that could have been done in the background into the STW time.

Is this left-over from the days we did not have server background GC? Should the condition for doing this be modified to only do this if we are doing a blocking GC? Or delete this completely?

[Maoni0]

since this was encapsulated in GcScanRoots and GC just calls it naturally called this. it's trying to distribute objects more evenly on different GC threads so we don't have one thread doing all the work that one object holds onto if it happened to be the one that got to it first. this was done before Background GC existed. clearly someone did this at some point and saw benefits. but that was a long time ago and we've added other ways to distribute the work.

if it's not hard to fix the lock I would go for that.

[jkotas]

if it's not hard to fix the lock I would go for that.

It should not be hard, but the changes in code that runs with runtime suspended tend to come with stress bug tail. The less structures we use while the runtime is suspended the better...

[Maoni0]

I'll talk to Peter about this.

[Maoni0]

fixed with PR#32795