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
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
