On Fri, 7 Apr 2023 19:17:46 GMT, Matias Saavedra Silva <matsa...@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