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

[Aleksey Shipilev]

On Thu, 15 Jun 2023 15:59:34 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)

Brief review. I realize more than half of my comments are for upstream PR. Reconcile them as you see fit.

src/hotspot/share/gc/g1/g1FullGCAdjustTask.cpp line 81:

> 79:     _collector(collector),
> 80:     _bitmap(collector->mark_bitmap()),
> 81:     _worker_id(worker_id) { }

Keep this in place, and just add `public:`/`private:` inline to hide the rest?

src/hotspot/share/gc/g1/g1FullGCAdjustTask.cpp line 111:

> 109:   marker->preserved_stack()->adjust_during_full_gc();
> 110: 
> 111:   G1AdjustClosure<ALT_FWD> adjust(collector());

Make a concession for code style, and name this local `_adjust`? Would save the renames everywhere else.

src/hotspot/share/gc/g1/g1FullGCCompactionPoint.inline.hpp line 34:

> 32: 
> 33: template<bool ALT_FWD>
> 34: void G1FullGCCompactionPoint::forward(oop object, size_t size) {

You moved this to get the template instantiations ready to use? You can leave (and probably should leave) the method in `.cpp`, and instead do the explicit instantiation there, like:


template G1FullGCCompactionPoint::forward<true>;
template G1FullGCCompactionPoint::forward<false>;

src/hotspot/share/gc/g1/g1FullGCCompactionPoint.inline.hpp line 66:

> 64:     }
> 65:     assert(object->forwardee() == NULL, "should be forwarded to NULL");
> 66:     */

Still need this commented out? One more reason not to move the code to `.inline.hpp`.

src/hotspot/share/gc/g1/g1FullGCOopClosures.inline.hpp line 68:

> 66: 
> 67: template<bool ALT_FWD>
> 68: template <class T> inline void G1AdjustClosure<ALT_FWD>::adjust_pointer(T* p) {

Suggestion:

template <bool ALT_FWD>
template <class T> inline void G1AdjustClosure<ALT_FWD>::adjust_pointer(T* p) {

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

> 382:   ShenandoahHeapRegionSet** const _worker_slices;
> 383: 
> 384:   template<bool ALT_FWD>

Again, keep this in place, do `public`/`private` declarations here. Would help Shenandoah backports.

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

> 795: 
> 796:   template<bool ALT_FWD>
> 797:   void work_impl(uint worker_id) {

`public`/`private` is cleaner than code moves here, again.

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

PR Review: https://git.openjdk.org/lilliput-jdk17u/pull/32#pullrequestreview-1486009390
PR Review Comment: https://git.openjdk.org/lilliput-jdk17u/pull/32#discussion_r1233920802
PR Review Comment: https://git.openjdk.org/lilliput-jdk17u/pull/32#discussion_r1233922994
PR Review Comment: https://git.openjdk.org/lilliput-jdk17u/pull/32#discussion_r1233927907
PR Review Comment: https://git.openjdk.org/lilliput-jdk17u/pull/32#discussion_r1233928704
PR Review Comment: https://git.openjdk.org/lilliput-jdk17u/pull/32#discussion_r1233929270
PR Review Comment: https://git.openjdk.org/lilliput-jdk17u/pull/32#discussion_r1233935035
PR Review Comment: https://git.openjdk.org/lilliput-jdk17u/pull/32#discussion_r1233935519