On Mon, 3 Apr 2023 03:32:27 GMT, Ioi Lam <ik...@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-me...@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
