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

[Thomas Schatzl]

On Wed, 27 Sep 2023 18:16:09 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>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.)
>

The title is still correct as mentioned in the other comment. This corroborates with the test results from the (now) attached test case.

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

Will fix.

> 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.

I used the same style as for the `CardSet` implementation. Fixed though because I do not care that much (although I do not really like the `nmethod**` everywhere instead).

> 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.)

something is missing here. You probably mean something like:

```     bool is_dead(G1CodeRootSetHashTableValue*) const```

We do not do that elsewhere either. We do not use `(...)` either elsewhere (in gc code at least), and I do not want to start this kind of discussion as part of this change.

> 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.

Every hashtable has its internal claim now. So to have (eventually) multiple threads help with a single hashtable, all threads will at least to visit and check the current claim value whether this particular hashtable has been fully claimed (processed).

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

PR Comment: https://git.openjdk.org/jdk/pull/15811#issuecomment-1739524187
PR Review Comment: https://git.openjdk.org/jdk/pull/15811#discussion_r1340324344
PR Review Comment: https://git.openjdk.org/jdk/pull/15811#discussion_r1340324873
PR Review Comment: https://git.openjdk.org/jdk/pull/15811#discussion_r1340325412