On Thu, 19 Jan 2023 17:23:19 GMT, Justin King <jck...@openjdk.org> wrote:
>> Remove abstraction that is a holdover from Solaris. Direct usages of >> `MmapArrayAllocator` have been switched to normal `malloc`. The >> justification is that none of the code paths are called from signal >> handlers, so using `mmap` directly does not make sense and is potentially >> slower than going through `malloc` which can potentially re-use memory >> without making any system calls. The remaining usages of `ArrayAllocator` >> and `MallocArrayAllocator` are equivalent. > > Justin King has updated the pull request incrementally with one additional > commit since the last revision: > > Do not pass nullptr to os::release_memory > > Signed-off-by: Justin King <jck...@google.com> Cursory review src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 125: > 123: return true; > 124: } > 125: Here, and in ZGC: we are cool with small allocations being rounded up to page size now? src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 131: > 129: void* const addr = os::reserve_memory(size, !ExecMem, mtGC); > 130: if (addr == nullptr) { > 131: return nullptr; reserve fails, we return null, commit fails, we exit? Why the inconsistency? src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 150: > 148: > 149: size_t G1CMMarkStack::size_for_array(size_t count) { > 150: return align_up(count * sizeof(TaskQueueEntryChunk), > os::vm_allocation_granularity()); assert against overflow src/hotspot/share/gc/z/zGranuleMap.inline.hpp line 79: > 77: template <typename T> > 78: size_t ZGranuleMap<T>::size_for_array(size_t count) { > 79: return align_up(count * sizeof(T), os::vm_allocation_granularity()); assert ag. overflow? src/hotspot/share/memory/padded.inline.hpp line 70: > 68: // Clear the allocated memory. > 69: memset(chunk, '\0', total_size); > 70: Old code, when doing the malloc path, did not initialize. Why here? test/lib-test/jdk/test/whitebox/vm_flags/SizeTTest.java line 38: > 36: > 37: public class SizeTTest { > 38: private static final String FLAG_NAME = > "StringDeduplicationCleanupDeadMinimum"; Small comment here that the flag itself, does not matter, just the fact that it is of type size_t? ------------- PR Review: https://git.openjdk.org/jdk/pull/11931#pullrequestreview-1437169771 PR Review Comment: https://git.openjdk.org/jdk/pull/11931#discussion_r1200844220 PR Review Comment: https://git.openjdk.org/jdk/pull/11931#discussion_r1200842961 PR Review Comment: https://git.openjdk.org/jdk/pull/11931#discussion_r1200845162 PR Review Comment: https://git.openjdk.org/jdk/pull/11931#discussion_r1200840143 PR Review Comment: https://git.openjdk.org/jdk/pull/11931#discussion_r1200851869 PR Review Comment: https://git.openjdk.org/jdk/pull/11931#discussion_r1200852952