Fix for solving lock contention issue in GC statics scanning.

[PeterSolMS]

I believe this is a better solution for GC statics scanning.

Instead of walking through the module list and scanning the statics for each module, walk the buckets in the large heap handle table where the statics are stored. This list is only changed in cooperative mode, so it should be safe to walk during GC suspensions.

[jkotas]

This looks good to me as an incremental improvement.

It still unfortunate that this is done during STW pause. I believe we should be taking every opportunity to move work from STW pauses to background.

[PeterSolMS]

Well, what we do during STW pause is essentially snapshotting the object pointers stored in the statics, we don't explore the object graph yet.

I think it's possible in principle to do the scanning while the program is running, this will likely need some care so we don't run into tricky race conditions.

[Maoni0]

great! this is a much better solution.

re switching orders in GCToEEInterface::GcScanRoots - I think in general this should be better except for the one scenario I pointed out (where a stack var holds onto the object array).

[Maoni0]

approved but I know Peter is working on a test to verify so there may (but unlikely) be some changes coming from that.

[VSadov]

I like this fix more than the previous one too.

[VSadov]

Re: doing the scan outside of STW. One way to do that could be storing the statics in managed arrays, - similar to what we do with collectible assemblies. Then scanning happens naturally.
That would be a feature work beyond the scope of a bug fix though.

[jkotas]

The statics are stored in managed arrays. This extra code frontloads scanning of these managed arrays with statics. It is not required for correctness - if it was deleted, everything would still correctly, just the timing would be different.

[VSadov]

The code mentions that this is not needed for collectible assemblies because they are stored in managed arrays, so I assumed that regular statics are not.

[VSadov]

In such case, perhaps enumerating just the arrays would be sufficient? - I.E. no need to dig into elements?

[jkotas]

You can make a similar argument for why this front-loading is beneficial at all. It is why it would be useful to have a micro-benchmark to demonstrate it. Then we can at least somewhat reason about the impact of different tweaks.

[VSadov]

Right. Enumerating just the containing arrays is basically the same as enumerating handles and we do that a few lines later.

My concern was that if multiple threads will start fight for every static variable, there could be some issues with false sharing. If you have N threads and K statics, it seems it would result in N*K accesses to the same data and in a worst case nearly all will be cache misses.

[PeterSolMS]

I wrote the micro-benchmark that Jan and Maoni suggested. Result is that there is significant benefit to the front-loading, but the handle table scanning is still the long pole in many cases. This is probably because if the thread doing the handle table scanning gets first to the array containing the statics, it will greedily mark the contained objects before actually tracing the contained object graphs. The other threads will then conclude there's nothing to do for them. Switching the order of handle table scanning and draining the mark list fixed this, but that is for a later checkin.

[jkotas]

Is this good to merge?

[Maoni0]

I think so unless @PeterSolMS has objections?