RFR: 8298048: Combine CDS archive heap into a single block [v2]

[Ioi Lam]

On Fri, 7 Apr 2023 19:17:46 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> Ioi Lam has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - more clean up: heap_regions -> heap_region, etc
>>  - @matias9927 comments
>
> 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

Fixed.

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

I fixed the assert.

> 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()?

Updated with the `MAX2()` macro.

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

I changed the code to pass "written" as a parameter similar to the other two calls. Also added comments.

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

I simplified the code -- there's only one range now so the start/end can be easily determined.

> 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())`

Fixed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163601901
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163601972
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163602280
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163602339
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163602400
PR Review Comment: https://git.openjdk.org/jdk/pull/13284#discussion_r1163602433