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

[Matias Saavedra Silva]

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

Changes requested by matsaave (Committer).

src/hotspot/share/cds/archiveBuilder.cpp line 1086:

> 1084:                          p2i(to_requested(start)), size_t(end - start));
> 1085:       log_data(start, end, to_requested(start), /*is_heap=*/true);
> 1086:     }

These log messages can be placed inside the else case before the break

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

> 367: template <typename T> void ArchiveHeapWriter::store_requested_oop_in_buffer(T* buffered_addr,
> 368:                                                                             oop request_oop) {
> 369:   //assert(is_in_requested_regions(request_oop), "must be");

Some left over commented code. I assume this should be removed or a new assert should be here to replace it.

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

> 527:     num_non_null_ptrs ++;
> 528: 
> 529:     if (max_idx < idx) {

Is there a built in min() function we can use here? Maybe std::min()?

src/hotspot/share/cds/filemap.cpp line 1674:

> 1672: 
> 1673:   char* buffer = NEW_C_HEAP_ARRAY(char, size_in_bytes, mtClassShared);
> 1674:   size_t written = write_bitmap(ptrmap, buffer, 0);

Maybe add a comment to clarify there is no offset? Constants in method parameters can be confusing sometimes.

src/hotspot/share/cds/filemap.cpp line 2035:

> 2033:     }
> 2034:     if (end < e) {
> 2035:       end = e;

Like mentioned before, maybe we have max() and min() methods to use here.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 520:

> 518:   } else {
> 519:     return true;
> 520:   }

Maybe make this `return reserved.contains(range.start()) && reserved.contains(range.last())`

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

PR Review: https://git.openjdk.org/jdk/pull/13284#pullrequestreview-1376508819
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1160911406
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1160911559
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1160913791
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1160916309
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1160916888
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1160924110