RFR: 8298048: Combine CDS archive heap into a single block

[Ashutosh Mehra]

On Mon, 3 Apr 2023 03:32:27 GMT, Ioi Lam <iklam at openjdk.org> wrote:

> This PR combines the "open" and "closed" regions of the CDS archive heap into a single region. This significantly simplifies the implementation, making it more compatible with non-G1 collectors. There's a net removal of ~1000 lines in src code and another ~1200 lines of tests.
> 
> **Notes for reviewers:**
> - Most of the code changes in CDS are removing the handling of "open" vs "closed" objects.
>   - Reviewers can start with ArchiveHeapWriter::copy_source_objs_to_buffer().
>   - It might be easier to see the diff with whitespaces off.
> - There are two major changes in the G1 code
>   - The archived objects are now stored in the "old" region (see g1CollectedHeap.cpp in [58d720e](https://github.com/openjdk/jdk/pull/13284/commits/58d720e294bb36f21cb88cddde724ed2b9e93770))
>   - The majority of the other changes are removal of the "archive" region type (see heapRegionType.hpp). For ease of review, such code is isolated in [a852dfb](https://github.com/openjdk/jdk/pull/13284/commits/a852dfbbf5ff56e035399f7cc3704f29e76697f6)
> - Testing changes:
>   - Now the archived java objects can move, along with the "old" regions that contain them. It's no longer possible to test whether a heap object came from CDS. As a result, the `WhiteBox.isShared(Object o)` API has been removed.
>   - Many tests that uses this API are removed. Most of them were written during early development of CDS archived objects and are no longer relevant.
> 
> **Testing:**
> - Mach5 tiers 1 ~ 7

Marked as reviewed by ashu-mehra at github.com (no known OpenJDK username).

cds changes look good! just few nitpicks.

src/hotspot/share/cds/archiveHeapLoader.cpp line 265:

> 263:                                            MemRegion& archive_space) {
> 264:   size_t total_bytes = 0;
> 265:   int i = MetaspaceShared::hp;

nitpick: this can be replaced with a better variable name instead of `i`, probably region_idx.

src/hotspot/share/cds/archiveHeapLoader.cpp line 274:

> 272:   assert(is_aligned(r->used(), HeapWordSize), "must be");
> 273:   total_bytes += r->used();
> 274:   loaded_region->_region_index = i;

nitpick: we can do away with `_region_index` and use `MetaspaceShared::hp` wherever required.

src/hotspot/share/cds/archiveHeapLoader.cpp line 445:

> 443:   }
> 444: 
> 445:   int i = MetaspaceShared::hp;

nitpick: same as before,  suggest to replace `i` with `region_idx`.

src/hotspot/share/cds/archiveHeapWriter.cpp line 54:

> 52: // The following are offsets from buffer_bottom()
> 53: size_t ArchiveHeapWriter::_buffer_used;
> 54: size_t ArchiveHeapWriter::_heap_roots_bottom;

nitpick: would be clearer if `_heap_roots_bottom` is named as `_heap_roots_bottom_offset`

src/hotspot/share/cds/metaspaceShared.hpp line 63:

> 61:     ro = 1,  // read-only shared space
> 62:     bm = 2,  // relocation bitmaps (freed after file mapping is finished)
> 63:     hp = 3,  // relocation bitmaps (freed after file mapping is finished)

This comment needs to be updated.

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

PR Review: https://git.openjdk.org/jdk/pull/13284#pullrequestreview-1380125495
PR Comment: https://git.openjdk.org/jdk/pull/13284#issuecomment-1504143568
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163361181
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163361230
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163361633
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163362914
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163362267