RFR: 8315503: G1: Code root scan causes long GC pauses due to imbalanced iteration [v6]

[Albert Mingkun Yang]

On Wed, 27 Sep 2023 07:23:39 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Hi all,
>> 
>>   please review this change that modifies the code root (remembered) set to use the CHT as internal representation.
>> 
>> This removes lots of locking (inhibiting throughput), provides automatic balancing for the code root scan phase, and (parallel) bulk unregistering of nmethdos during code cache unloading improving performance of various pauses that deal with code root sets.
>> 
>> With a stress test that frequently loads and unloads 6000 classes and associated methods from them we could previously see the following issues:
>> 
>> During collection pauses:
>> 
>> [4179,965s][gc,phases ] GC(273) Evacuate Collection Set: 812,18ms
>> [..]
>> [4179,965s][gc,phases ] GC(273) Code Root Scan (ms): Min: 0,00, Avg: 59,03, Max: 775,12, Diff: 775,12, Sum: 944,44, Workers: 16
>> [...]
>> [4179,965s][gc,phases ] GC(273) Termination (ms): Min: 0,03, Avg: 643,90, Max: 690,96, Diff: 690,93, Sum: 10302,47, Workers: 16
>> 
>> 
>> Code root scan now reduces to ~22ms max on average in this case.
>> 
>> Class unloading (breaking down the code cache flushing, i.e. `CodeCache::flush_unlinked_nmethods`):
>> 
>> Clear Exception Caches   35,5ms
>> Unregister NMethods     598,5ms                                <---- this is nmethod unregistering.
>> Unregister Old NMethods   3,0ms
>> CodeBlob flush           41,1ms
>> CodeCache free         5730,3ms
>> 
>> 
>> With this change, the `unregister nmethods` phase takes ~25ms max on that stress test. @walulyai contributed this part.
>> 
>> We have recently seen some imbalances in code root scan and long Remark pauses (thankfully not to that extreme) in other real-world applications too:
>> 
>> [2466.979s][gc,phases      ] GC(131)     Code Root Scan (ms):           Min:  0.0, Avg:  5.7, Max: 46.4, Diff: 46.4, Sum: 57.0, Workers: 10
>> 
>> 
>> Some random comment:
>> * the mutex for the CHT had to be decreased in priority by one to not conflict with `CodeCache_lock`. This does not seem to be detrimental otherwise. At the same time, I had to move the locks at `nosafepoint-3` to `nosafepoint-4` as well to keep previous ordering. All mutexes with uses of `nosafepoint` as their rank seem to be good now.
>> 
>> Testing: tier1-5
>> 
>> Thanks,
>>   Thomas
>
> Thomas Schatzl has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Split off bulk removal of dead nmethods from code root sets
>  - Initial comments from albert

The title should probably be revised, sth like "use conc-hashtable for code-root", to better match the content of the PR. (For example, I don't get the "imbalanced" part after going through the diff.)

Could you also update the perf number now that only one optimization is included?

src/hotspot/share/gc/g1/g1CodeRootSet.cpp line 45:

> 43: class G1CodeRootSetHashTableConfig : public StackObj {
> 44: public:
> 45:   using Value = G1CodeRootSetHashTableValue;

Could one use `nmethod*` here directly? Having one extra indirection/layer makes it harder to follow.

src/hotspot/share/gc/g1/g1CodeRootSet.cpp line 82:

> 80:     uintx get_hash() const;
> 81:     bool equals(G1CodeRootSetHashTableValue* value);
> 82:     bool is_dead(G1CodeRootSetHashTableValue* value) const { return false; }

I wonder if `(...)` works, since the arg in unused. (Inspired by `struct NOP` in conc-hashtable.)

src/hotspot/share/gc/g1/g1RemSet.cpp line 825:

> 823: 
> 824:     // Scan code root remembered sets.
> 825:     {

Without the claim-logic, all workers will scan code-root. Why is it needed that multiples workers scan the same set of code-root repeatedly? I thought once is enough per region.

-------------

PR Review: https://git.openjdk.org/jdk/pull/15811#pullrequestreview-1647285080
PR Review Comment: https://git.openjdk.org/jdk/pull/15811#discussion_r1339005262
PR Review Comment: https://git.openjdk.org/jdk/pull/15811#discussion_r1339007198
PR Review Comment: https://git.openjdk.org/jdk/pull/15811#discussion_r1339017373