RFR: 8344883: Do not use mtNone if we know the tag type [v2]
Stefan Karlsson
stefank at openjdk.org
Fri Mar 28 08:28:23 UTC 2025
On Thu, 27 Mar 2025 19:47:56 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:
>> This is a follow-up to #21843. Here we are focusing on removing the mem tag paremeter with default value of mtNone, to force everyone to provide mem tag, if known.
>>
>> I tried to fill in tag, when I was pretty certain that I had the right type.
>>
>> At least one more follow-up will be needed after this, to change the remaining mtNone to valid values.
>
> Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision:
>
> work
I went over the patch and added suggestions for places where I think you're using the wrong tag, or where I think it is obvious that there's a better tag than mtNone. I've also suggested removal of the now redundant 'executable' argument, which I want to see as little of as possible given that it is a wart on the memory reservation APIs (IMHO).
src/hotspot/os/bsd/gc/z/zPhysicalMemoryBacking_bsd.cpp line 81:
> 79:
> 80: // Reserve address space for backing memory
> 81: _base = (uintptr_t)os::reserve_memory(max_capacity, mtJavaHeap, false);
Suggestion:
_base = (uintptr_t)os::reserve_memory(max_capacity, mtJavaHeap);
src/hotspot/os/windows/os_windows.cpp line 3261:
> 3259: assert(extra_size >= size, "overflow, size is too large to allow alignment");
> 3260:
> 3261:
Suggestion:
src/hotspot/os/windows/os_windows.cpp line 3267:
> 3265: for (int attempt = 0; attempt < max_attempts && aligned_base == nullptr; attempt ++) {
> 3266: char* extra_base = file_desc != -1 ? os::map_memory_to_file(extra_size, file_desc, mem_tag) :
> 3267: os::reserve_memory(extra_size, mem_tag, false);
Suggestion:
os::reserve_memory(extra_size, mem_tag);
src/hotspot/os/windows/os_windows.cpp line 3284:
> 3282: // Which may fail, hence the loop.
> 3283: aligned_base = file_desc != -1 ? os::attempt_map_memory_to_file_at(aligned_base, size, file_desc, mem_tag) :
> 3284: os::attempt_reserve_memory_at(aligned_base, size, mem_tag, false);
Suggestion:
os::attempt_reserve_memory_at(aligned_base, size, mem_tag);
src/hotspot/os/windows/perfMemory_windows.cpp line 57:
> 55:
> 56: // allocate an aligned chuck of memory
> 57: char* mapAddress = os::reserve_memory(size, mtNone);
To match with the other platforms:
Suggestion:
char* mapAddress = os::reserve_memory(size, mtInternal);
src/hotspot/share/classfile/compactHashtable.cpp line 229:
> 227: quit("Unable to open hashtable dump file", filename);
> 228: }
> 229: _base = os::map_memory(_fd, filename, 0, nullptr, _size, mtSymbol, true, false);
This seems to be used to read Symbols *OR* String. This probably needs to be something else. I suggest to revert to mtNone and figure out the appropriate tag later.
Suggestion:
_base = os::map_memory(_fd, filename, 0, nullptr, _size, mtNone, true, false);
src/hotspot/share/gc/parallel/parMarkBitMap.cpp line 52:
> 50: rs_align,
> 51: page_sz,
> 52: mtNone);
Suggestion:
mtGC);
src/hotspot/share/gc/shenandoah/shenandoahCardTable.cpp line 62:
> 60: _write_byte_map_base = _byte_map_base;
> 61:
> 62: ReservedSpace read_space = MemoryReserver::reserve(_byte_map_size, rs_align, _page_size, mtNone);
Suggestion:
ReservedSpace read_space = MemoryReserver::reserve(_byte_map_size, rs_align, _page_size, mtGC);
src/hotspot/share/memory/allocation.inline.hpp line 61:
> 59: size_t size = size_for(length);
> 60:
> 61: char* addr = os::reserve_memory(size, mem_tag, !ExecMem);
Suggestion:
char* addr = os::reserve_memory(size, mem_tag);
src/hotspot/share/memory/allocation.inline.hpp line 78:
> 76: size_t size = size_for(length);
> 77:
> 78: char* addr = os::reserve_memory(size, mem_tag, !ExecMem);
Suggestion:
char* addr = os::reserve_memory(size, mem_tag);
src/hotspot/share/memory/metaspace/testHelpers.cpp line 85:
> 83: if (reserve_limit > 0) {
> 84: // have reserve limit -> non-expandable context
> 85: _rs = MemoryReserver::reserve(reserve_limit * BytesPerWord, Metaspace::reserve_alignment(), os::vm_page_size(), mtNone);
Suggestion:
_rs = MemoryReserver::reserve(reserve_limit * BytesPerWord, Metaspace::reserve_alignment(), os::vm_page_size(), mtTest);
src/hotspot/share/memory/metaspace/virtualSpaceNode.cpp line 259:
> 257: ReservedSpace rs = MemoryReserver::reserve(word_size * BytesPerWord,
> 258: Settings::virtual_space_node_reserve_alignment_words() * BytesPerWord,
> 259: os::vm_page_size(), mtNone);
Suggestion:
os::vm_page_size(), mtMetaspace);
src/hotspot/share/prims/jni.cpp line 2403:
> 2401: if (bad_address == nullptr) {
> 2402: size_t size = os::vm_allocation_granularity();
> 2403: bad_address = os::reserve_memory(size, mtInternal, false);
Suggestion:
bad_address = os::reserve_memory(size, mtInternal);
src/hotspot/share/prims/whitebox.cpp line 720:
> 718:
> 719: WB_ENTRY(jlong, WB_NMTReserveMemory(JNIEnv* env, jobject o, jlong size))
> 720: return (jlong)(uintptr_t)os::reserve_memory(size, mtTest, false);
Suggestion:
return (jlong)(uintptr_t)os::reserve_memory(size, mtTest);
src/hotspot/share/prims/whitebox.cpp line 724:
> 722:
> 723: WB_ENTRY(jlong, WB_NMTAttemptReserveMemoryAt(JNIEnv* env, jobject o, jlong addr, jlong size))
> 724: return (jlong)(uintptr_t)os::attempt_reserve_memory_at((char*)(uintptr_t)addr, (size_t)size, mtTest, false);
Suggestion:
return (jlong)(uintptr_t)os::attempt_reserve_memory_at((char*)(uintptr_t)addr, (size_t)size, mtTest);
src/hotspot/share/prims/whitebox.cpp line 1515:
> 1513: static volatile char* p;
> 1514:
> 1515: p = os::reserve_memory(os::vm_allocation_granularity(), mtSymbol);
Suggestion:
p = os::reserve_memory(os::vm_allocation_granularity(), mtTest);
src/hotspot/share/runtime/os.cpp line 2130:
> 2128: log_trace(os, map)(ERRFMT, ERRFMTARGS);
> 2129: log_debug(os, map)("successfully attached at " PTR_FORMAT, p2i(result));
> 2130: MemTracker::record_virtual_memory_reserve((address)result, bytes, CALLER_PC, mtNone);
I think attempt_reserve_memory_between should provide the correct mem tag.
src/hotspot/share/runtime/os.cpp line 2336:
> 2334: if (result != nullptr) {
> 2335: // The memory is committed
> 2336: MemTracker::record_virtual_memory_reserve_and_commit((address)result, size, CALLER_PC, mtNone);
reserve_memory_special should take a mem tag, but I guess you intend to do that as a follow-up RFE?
src/hotspot/share/runtime/safepointMechanism.cpp line 60:
> 58: const size_t page_size = os::vm_page_size();
> 59: const size_t allocation_size = 2 * page_size;
> 60: char* polling_page = os::reserve_memory(allocation_size, mtSafepoint, !ExecMem);
Suggestion:
char* polling_page = os::reserve_memory(allocation_size, mtSafepoint);
src/hotspot/share/utilities/debug.cpp line 715:
> 713: #ifdef CAN_SHOW_REGISTERS_ON_ASSERT
> 714: void initialize_assert_poison() {
> 715: char* page = os::reserve_memory(os::vm_page_size(), mtInternal, !ExecMem);
Suggestion:
char* page = os::reserve_memory(os::vm_page_size(), mtInternal);
test/hotspot/gtest/gc/g1/test_stressCommitUncommit.cpp line 86:
> 84: os::vm_allocation_granularity(),
> 85: os::vm_page_size(),
> 86: mtNone);
Suggestion:
mtTest);
test/hotspot/gtest/gc/g1/test_stressCommitUncommit.cpp line 113:
> 111: os::vm_allocation_granularity(),
> 112: os::vm_page_size(),
> 113: mtNone);
Suggestion:
mtTest);
test/hotspot/gtest/memory/test_virtualspace.cpp line 62:
> 60: ASSERT_PRED2(is_size_aligned, size, os::vm_allocation_granularity());
> 61:
> 62: ReservedSpace rs = MemoryReserver::reserve(size, mtNone);
Why did you do this change?
Suggestion:
ReservedSpace rs = MemoryReserver::reserve(size, mtTest);
test/hotspot/gtest/memory/test_virtualspace.cpp line 76:
> 74: ASSERT_PRED2(is_size_aligned, size, alignment) << "Incorrect input parameters";
> 75: size_t page_size = UseLargePages ? os::large_page_size() : os::vm_page_size();
> 76: ReservedSpace rs = MemoryReserver::reserve(size, alignment, page_size, mtNone);
Suggestion:
ReservedSpace rs = MemoryReserver::reserve(size, alignment, page_size, mtTest);
test/hotspot/gtest/memory/test_virtualspace.cpp line 104:
> 102: size_t page_size = large ? os::large_page_size() : os::vm_page_size();
> 103:
> 104: ReservedSpace rs = MemoryReserver::reserve(size, alignment, page_size, mtNone);
Suggestion:
ReservedSpace rs = MemoryReserver::reserve(size, alignment, page_size, mtTest);
test/hotspot/gtest/memory/test_virtualspace.cpp line 215:
> 213: case Default:
> 214: case Reserve:
> 215: return MemoryReserver::reserve(reserve_size_aligned, mtNone);
Suggestion:
return MemoryReserver::reserve(reserve_size_aligned, mtTest);
test/hotspot/gtest/memory/test_virtualspace.cpp line 221:
> 219: os::vm_allocation_granularity(),
> 220: os::vm_page_size(),
> 221: mtNone);
Suggestion:
mtTest);
test/hotspot/gtest/memory/test_virtualspace.cpp line 300:
> 298: size_t large_page_size = os::large_page_size();
> 299:
> 300: ReservedSpace reserved = MemoryReserver::reserve(large_page_size, large_page_size, large_page_size, mtNone);
Suggestion:
ReservedSpace reserved = MemoryReserver::reserve(large_page_size, large_page_size, large_page_size, mtTest);
test/hotspot/gtest/memory/test_virtualspace.cpp line 370:
> 368: alignment,
> 369: page_size,
> 370: mtNone);
Suggestion:
mtTest);
test/hotspot/gtest/memory/test_virtualspace.cpp line 388:
> 386: ASSERT_TRUE(is_aligned(size, os::vm_allocation_granularity())) << "Must be at least AG aligned";
> 387:
> 388: ReservedSpace rs = MemoryReserver::reserve(size, mtNone);
Suggestion:
ReservedSpace rs = MemoryReserver::reserve(size, mtTest);
test/hotspot/gtest/memory/test_virtualspace.cpp line 416:
> 414: alignment,
> 415: page_size,
> 416: mtNone);
Suggestion:
mtTest);
test/hotspot/gtest/memory/test_virtualspace.cpp line 521:
> 519: case Reserve:
> 520: return MemoryReserver::reserve(reserve_size_aligned,
> 521: mtNone);
Suggestion:
mtTest);
test/hotspot/gtest/memory/test_virtualspace.cpp line 527:
> 525: os::vm_allocation_granularity(),
> 526: os::vm_page_size(),
> 527: mtNone);
Suggestion:
mtTest);
test/hotspot/gtest/memory/test_virtualspace.cpp line 585:
> 583: large_page_size,
> 584: large_page_size,
> 585: mtNone);
Suggestion:
mtTest);
test/hotspot/gtest/nmt/test_nmt_locationprinting.cpp line 116:
> 114:
> 115: static void test_for_mmap(size_t sz, ssize_t offset) {
> 116: char* addr = os::reserve_memory(sz, mtTest, false);
Suggestion:
char* addr = os::reserve_memory(sz, mtTest);
test/hotspot/gtest/runtime/test_committed_virtualmemory.cpp line 94:
> 92: const size_t page_sz = os::vm_page_size();
> 93: const size_t size = num_pages * page_sz;
> 94: char* base = os::reserve_memory(size, mtThreadStack, !ExecMem);
Suggestion:
char* base = os::reserve_memory(size, mtThreadStack);
test/hotspot/gtest/runtime/test_committed_virtualmemory.cpp line 162:
> 160: const size_t num_pages = 4;
> 161: const size_t size = num_pages * page_sz;
> 162: char* base = os::reserve_memory(size, mtTest, !ExecMem);
Suggestion:
char* base = os::reserve_memory(size, mtTest);
test/hotspot/gtest/runtime/test_committed_virtualmemory.cpp line 208:
> 206: const size_t size = num_pages * page_sz;
> 207:
> 208: char* base = os::reserve_memory(size, mtTest, !ExecMem);
Suggestion:
char* base = os::reserve_memory(size, mtTest);
test/hotspot/gtest/runtime/test_os.cpp line 261:
> 259: // two pages, first one protected.
> 260: const size_t ps = os::vm_page_size();
> 261: char* two_pages = os::reserve_memory(ps * 2, mtTest, false);
Suggestion:
char* two_pages = os::reserve_memory(ps * 2, mtTest);
test/hotspot/gtest/runtime/test_os.cpp line 533:
> 531: size_t total_range_len = num_stripes * stripe_len;
> 532: // Reserve a large contiguous area to get the address space...
> 533: p = (address)os::reserve_memory(total_range_len, mtNone);
Suggestion:
p = (address)os::reserve_memory(total_range_len, mtTest);
test/hotspot/gtest/runtime/test_os.cpp line 547:
> 545: const bool executable = stripe % 2 == 0;
> 546: #endif
> 547: q = (address)os::attempt_reserve_memory_at((char*)q, stripe_len, mtNone, executable);
Suggestion:
q = (address)os::attempt_reserve_memory_at((char*)q, stripe_len, mtTest, executable);
test/hotspot/gtest/runtime/test_os.cpp line 567:
> 565: assert(is_aligned(stripe_len, os::vm_allocation_granularity()), "Sanity");
> 566: size_t total_range_len = num_stripes * stripe_len;
> 567: address p = (address)os::reserve_memory(total_range_len, mtNone);
Suggestion:
address p = (address)os::reserve_memory(total_range_len, mtTest);
test/hotspot/gtest/runtime/test_os.cpp line 634:
> 632:
> 633: // ...re-reserve the middle stripes. This should work unless release silently failed.
> 634: address p2 = (address)os::attempt_reserve_memory_at((char*)p_middle_stripes, middle_stripe_len, mtNone);
Suggestion:
address p2 = (address)os::attempt_reserve_memory_at((char*)p_middle_stripes, middle_stripe_len, mtTest);
test/hotspot/gtest/runtime/test_os.cpp line 657:
> 655: TEST_VM(os, release_bad_ranges) {
> 656: #endif
> 657: char* p = os::reserve_memory(4 * M, mtNone);
Suggestion:
char* p = os::reserve_memory(4 * M, mtTest);
test/hotspot/gtest/runtime/test_os.cpp line 692:
> 690: // // make things even more difficult by trying to reserve at the border of the region
> 691: address border = p + num_stripes * stripe_len;
> 692: address p2 = (address)os::attempt_reserve_memory_at((char*)border, stripe_len, mtNone);
Suggestion:
address p2 = (address)os::attempt_reserve_memory_at((char*)border, stripe_len, mtTest);
test/hotspot/gtest/runtime/test_os.cpp line 733:
> 731: // Reserve a small range and fill it with a marker string, should show up
> 732: // on implementations displaying range snippets
> 733: char* p = os::reserve_memory(1 * M, mtInternal, false);
Suggestion:
char* p = os::reserve_memory(1 * M, mtInternal);
test/hotspot/gtest/runtime/test_os.cpp line 757:
> 755: // A simple allocation
> 756: {
> 757: address p = (address)os::reserve_memory(total_range_len, mtNone);
Suggestion:
address p = (address)os::reserve_memory(total_range_len, mtTest);
test/hotspot/gtest/runtime/test_os.cpp line 1062:
> 1060:
> 1061: TEST_VM(os, reserve_at_wish_address_shall_not_replace_mappings_smallpages) {
> 1062: char* p1 = os::reserve_memory(M, mtTest, false);
Suggestion:
char* p1 = os::reserve_memory(M, mtTest);
test/hotspot/gtest/runtime/test_os.cpp line 1072:
> 1070: if (UseLargePages && !os::can_commit_large_page_memory()) { // aka special
> 1071: const size_t lpsz = os::large_page_size();
> 1072: char* p1 = os::reserve_memory_aligned(lpsz, lpsz, mtTest, false);
Suggestion:
char* p1 = os::reserve_memory_aligned(lpsz, lpsz, mtTest);
test/hotspot/gtest/runtime/test_os.cpp line 1098:
> 1096: const size_t size = pages * page_sz;
> 1097:
> 1098: char* base = os::reserve_memory(size, mtTest, false);
Suggestion:
char* base = os::reserve_memory(size, mtTest);
test/hotspot/gtest/runtime/test_os_linux.cpp line 357:
> 355: const bool useThp = UseTransparentHugePages;
> 356: UseTransparentHugePages = true;
> 357: char* const heap = os::reserve_memory(size, mtInternal, false);
Suggestion:
char* const heap = os::reserve_memory(size, mtInternal);
-------------
Changes requested by stefank (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/24282#pullrequestreview-2724588390
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018111000
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018112894
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018113093
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018113247
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018114051
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018121233
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018121891
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018122312
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018122564
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018122715
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018123591
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018124074
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018125155
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018125568
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018125876
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018126443
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018128479
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018129358
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018098166
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018098617
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018098956
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018099157
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018100108
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018100521
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018100717
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018100870
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018101049
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018101200
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018101373
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018101541
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018101692
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018101911
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018102064
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018102298
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018102633
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018103596
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018103912
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018104093
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018104376
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018104743
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018105030
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018105216
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018105410
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018105552
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018105686
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018105792
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018105928
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018106123
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018106324
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018106474
PR Review Comment: https://git.openjdk.org/jdk/pull/24282#discussion_r2018107303
More information about the hotspot-dev
mailing list