[master] RFR: 8304710: [Lilliput] Use forwarding table for sliding GCs [v3]

Thu Apr 13 19:47:58 UTC 2023

On Thu, 13 Apr 2023 12:03:39 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> This changes the full-GC forwarding of Serial, Parallel, G1 and Shenandoah to use a forwarding table instead of the fancy SlidingForwarding that we had until now. SlidingForwarding has been problematic because it did not work universally, in particular we needed to disable the G1 serial full GC (the last last ditch GC, after parallel full GC failed). Also, sliding forwarding would not work when we start working towards 32bit headers.
>> 
>> The implementation of the forwarding table is a dense array of forwardings, with sorted entries and binary search. For details, have a look at the comments at the top of class ForwardingTable.
>> 
>> The change also greatly reduces the upstream diff.
>> 
>> In G1 I added a little code to count the marked objects in each region by counting bits in the mark-bitmap.
>> 
>> In Serial GC I likewise added some code to count new vs old objects during full-GC marking. Here there would be the alternative to treat the heap as a single region, and count marked objects without distinguishing old vs young. The problem is that old is allocated in higher memory than young, and the full-GC compacts old-gen first, and then young-gen, which breaks the assumption in ForwardingTable, which leads to very slow forwarding insertions. I also tried to simply swap the old and young reserved memory, and that is possible, but triggers all sorts of related changes where the memory order is implicitely used for stuff like is_in_young() etc.
>> 
>> Testing:
>>  - [x] tier1
>>  - [x] tier2
>
> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Ensure compaction order in serial GC

Cursory review. I have not got into the thick of how the forwarding tables are working, I'll do it in the second round.

src/hotspot/share/gc/serial/markSweep.cpp line 211:

> 209:   } else {
> 210:     _old_marked_objects++;
> 211:   }

Is this supposed to be thread-safe? Or we don't care, because it is Serial?

src/hotspot/share/gc/shared/forwardingTable.cpp line 1:

> 1: 

Critical bug: excess newline.

src/hotspot/share/gc/shared/forwardingTable.cpp line 57:

> 55: }
> 56: 
> 57: void ForwardingTable::begin_region(size_t idx, size_t num_forwardings) {

Should assert `idx < _max_regions` here?

src/hotspot/share/gc/shared/forwardingTable.hpp line 34:

> 32: // TODO: Address range and num-regions permitting, we could switch to
> 33: // a more compact encoding:
> 34: // - N bits to encode number of words from start of from region

Suggestion:

// - N bits to encode number of words from start of from-region

src/hotspot/share/gc/shared/forwardingTable.hpp line 37:

> 35: // - M bits to encode number of words from start of to-region
> 36: // - X bits to encode to-region index
> 37: // from-region index is known implicitely.

Suggestion:

// from-region index is known implicitly.

src/hotspot/share/gc/shared/forwardingTable.hpp line 107:

> 105:  *   begin_region(). This initializes the corresponding per-region table.
> 106:  * - Insert and look-up forwardings.
> 107:  * - When forwaring is finished, call end(). This will dispose all internal data structures.

Suggestion:

 * - When forwarding is finished, call end(). This will dispose all internal data structures.

src/hotspot/share/gc/shared/forwardingTable.hpp line 127:

> 125: };
> 126: 
> 127: #endif // SHARE_GC_SHARED_FORWARDINGABLE_HPP

Suggestion:

#endif // SHARE_GC_SHARED_FORWARDINGTABLE_HPP

src/hotspot/share/gc/shared/forwardingTable.inline.hpp line 37:

> 35: }
> 36: 
> 37: // When found, returns theindex into _table

Suggestion:

// When found, returns the index into _table

src/hotspot/share/gc/shared/gcForwarding.hpp line 40:

> 38: public:
> 39: 
> 40:   static void initialize(AddrToIdxFn addr_to_idx, size_t max_regions);

Suggestion:

private:
  static ForwardingTable* _forwarding_table;

public:
  static void initialize(AddrToIdxFn addr_to_idx, size_t max_regions);

src/hotspot/share/gc/shared/gcForwarding.hpp line 45:

> 43:   static void end();
> 44: 
> 45:   static inline bool is_forwarded(oop obj);

I'd suggest additional `is_not_forwarded`, so that we don't have to invert `!` and `!=` on some paths.

src/hotspot/share/gc/shared/gcForwarding.inline.hpp line 54:

> 52:     assert(_forwarding_table != nullptr, "expect forwarding table initialized");
> 53:     _forwarding_table->forward_to(obj, fwd);
> 54:     assert(is_forwarded(obj), "must be forwarded");

This assert looks redundant, since the next one would test it implicitly.

src/hotspot/share/gc/shared/genCollectedHeap.cpp line 108:

> 106:   auto addr_to_idx = [](const void* addr) {
> 107:     if (GenCollectedHeap::heap()->is_in_young(addr)) return (size_t)0;
> 108:     else return (size_t)1;

Suggestion:

    if (GenCollectedHeap::heap()->is_in_young(addr)) {
      return (size_t)0;
    } else {
      return (size_t)1;
    }

test/hotspot/gtest/gc/shared/test_preservedMarks.cpp line 59:

> 57: 
> 58:   bool old_compact_headers = UseCompactObjectHeaders;
> 59:   UseCompactObjectHeaders = false;

FlagSetting fs(UseCompactObjectHeaders, false);

?

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

PR Review: https://git.openjdk.org/lilliput/pull/83#pullrequestreview-1383531256
PR Review Comment: https://git.openjdk.org/lilliput/pull/83#discussion_r1165599869
PR Review Comment: https://git.openjdk.org/lilliput/pull/83#discussion_r1165603072
PR Review Comment: https://git.openjdk.org/lilliput/pull/83#discussion_r1165666592
PR Review Comment: https://git.openjdk.org/lilliput/pull/83#discussion_r1165640939
PR Review Comment: https://git.openjdk.org/lilliput/pull/83#discussion_r1165641371
PR Review Comment: https://git.openjdk.org/lilliput/pull/83#discussion_r1165643213
PR Review Comment: https://git.openjdk.org/lilliput/pull/83#discussion_r1165672770
PR Review Comment: https://git.openjdk.org/lilliput/pull/83#discussion_r1165643947
PR Review Comment: https://git.openjdk.org/lilliput/pull/83#discussion_r1165658239
PR Review Comment: https://git.openjdk.org/lilliput/pull/83#discussion_r1165659916
PR Review Comment: https://git.openjdk.org/lilliput/pull/83#discussion_r1165661823
PR Review Comment: https://git.openjdk.org/lilliput/pull/83#discussion_r1165662664
PR Review Comment: https://git.openjdk.org/lilliput/pull/83#discussion_r1165782919