[lilliput-jdk17u:lilliput] RFR: 8310156: [Lilliput/JDK17] Specialize full-GC loops [v2]

[Aleksey Shipilev]

On Mon, 19 Jun 2023 15:25:51 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> We already ported most of [JDK-8305896](https://bugs.openjdk.org/browse/JDK-8305896) to Lilliput/JDK17. What's missing is the specialization of the full-GC loops so that performance on the legacy path is not impacted and performance on the alt-GC-forwarding path is minimized.
>> 
>> This corresponds to: https://github.com/openjdk/jdk/pull/13582/commits/6bbd29525b2864be37d96750ddc78f73d8eb0b65
>> 
>> However, the changes differ significantly:
>> - G1 serial compaction is different
>> - There is no humongous compaction in JDK17 G1
>> - Serial GC has seen some refactors of the relevant loops between JDK17 and JDK22
>> 
>> I see failing tests with Shenandoah, but they are pre-existing and should be investigated separately.
>> 
>> Testing:
>>  - [x] hotspot_gc
>>  - [x] hotspot_gc -UCOH
>>  - [x] tier1 -XX:+UseG1GC
>>  - [x] tier1 -XX:+UseSerialGC
>>  - [x] tier1 -XX:+UseShenandoahGC (not clean, but no regressions, either)
>>  - [x] tier1 -XX:+UseG1GC -UCOH
>>  - [x] tier1 -XX:+UseSerialGC -UCOH
>>  - [x] tier1 -XX:+UseShenandoahGC -UCOH (not clean, but no regressions, either)
>
> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
> 
>   @shipilev's comments

More comments. It feels like all this template mess is quite intrusive, but I cannot find a better way to do this :/ Anyway, this gets us closer:

src/hotspot/share/gc/g1/g1FullCollector.cpp line 33:

> 31: #include "gc/g1/g1FullGCAdjustTask.hpp"
> 32: #include "gc/g1/g1FullGCCompactTask.hpp"
> 33: #include "gc/g1/g1FullGCCompactionPoint.hpp"

This include is still needed, or?

src/hotspot/share/gc/g1/g1FullGCPrepareTask.hpp line 58:

> 56:   class G1CalculatePointersClosure : public HeapRegionClosure {
> 57:   private:
> 58:     template <bool is_humongous>

Unnecessary change?

src/hotspot/share/gc/serial/markSweep.hpp line 139:

> 137:                                 // Save the mark word so it can be restored later
> 138:   template <bool ALT_FWD>
> 139:   static void adjust_marks();   // Adjust the pointers in the preserved marks table

This feels like `adjust_marks_impl`? Because it nominally clashes with existing non-templated `adjust_marks` a line below?

src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 832:

> 830:   PreservedMarksSet* _preserved_marks;
> 831: 
> 832:   template <bool ALT_FWD>

Keep this one in place, like others: use `public`/`private` to scope it right.

src/hotspot/share/gc/shenandoah/shenandoahFullGC.cpp line 923:

> 921:   }
> 922: public:
> 923:   ShenandoahCompactObjectsTask(ShenandoahHeapRegionSet** worker_slices) :

Same, keep it in place?

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

PR Review: https://git.openjdk.org/lilliput-jdk17u/pull/32#pullrequestreview-1487682460
PR Review Comment: https://git.openjdk.org/lilliput-jdk17u/pull/32#discussion_r1234974824
PR Review Comment: https://git.openjdk.org/lilliput-jdk17u/pull/32#discussion_r1234985003
PR Review Comment: https://git.openjdk.org/lilliput-jdk17u/pull/32#discussion_r1234989652
PR Review Comment: https://git.openjdk.org/lilliput-jdk17u/pull/32#discussion_r1234993926
PR Review Comment: https://git.openjdk.org/lilliput-jdk17u/pull/32#discussion_r1234994390