RFR: 8330076: [NMT] add/make a mandatory MEMFLAGS argument to family of os::reserve/commit/uncommit memory API [v4]

[Afshin Zafari]

On Fri, 12 Apr 2024 07:06:50 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   review comments applied.
>
> src/hotspot/os/windows/os_windows.cpp line 3137:
> 
>> 3135:   // If reservation failed, return null
>> 3136:   if (p_buf == nullptr) return nullptr;
>> 3137:   MemTracker::record_virtual_memory_reserve((address)p_buf, size_of_reserve, CALLER_PC, mtInternal);
> 
> I think that allocate_pages_individually should take a MEMFLAGS argument instead of using mtInternal here.

It takes now. All corresponding functions in call hierarchy are also changed.

> src/hotspot/os/windows/os_windows.cpp line 3198:
> 
>> 3196:         // the release.
>> 3197:         MemTracker::record_virtual_memory_reserve((address)p_buf,
>> 3198:                                                   bytes_to_release, CALLER_PC, mtNone);
> 
> I don't think we should ever use `mtNone` in code outside of the NMT code. If you follow my suggestion above that allocate_pages_individually should take a MEMFLAG arg, then it could be used here.

Corrected.

> src/hotspot/os/windows/os_windows.cpp line 3218:
> 
>> 3216:     MemTracker::record_virtual_memory_reserve_and_commit((address)p_buf, bytes, CALLER_PC);
>> 3217:   } else {
>> 3218:     MemTracker::record_virtual_memory_reserve((address)p_buf, bytes, CALLER_PC, mtNone);
> 
> Use the correct MEMFLAG here instead of mtNone.

Done.

> src/hotspot/os/windows/os_windows.cpp line 3771:
> 
>> 3769:   if (!is_committed) {
>> 3770:     commit_memory_or_exit(addr, bytes, prot == MEM_PROT_RWX,
>> 3771:                           "cannot commit protection page", mtNone);
> 
> This should probably be something else than mtNone.

Changed to `mtInternal`. When `protect_memory` is called with uncommitted area (`is_committed == false`), the flag is mtInternal.

> src/hotspot/share/jfr/recorder/storage/jfrVirtualMemory.cpp line 107:
> 
>> 105:   _rs = ReservedSpace(reservation_size_request_bytes,
>> 106:                       os::vm_allocation_granularity(),
>> 107:                       os::vm_page_size(), mtTracing);
> 
> The mtTracing should probably be on a separate line, so that it follows the style of the surrounding code.

Done.

> src/hotspot/share/memory/virtualspace.cpp line 45:
> 
>> 43: // Dummy constructor
>> 44: ReservedSpace::ReservedSpace() : _base(nullptr), _size(0), _noaccess_prefix(0),
>> 45:     _alignment(0), _special(false), _fd_for_heap(-1), _nmt_flag(mtNone), _executable(false) {
> 
> In almost all code we pass in the executable before the flag, but in ReservedSpace the flag is located before the executable. I think it would be nice to flip the order in this class. I understand that _executable is in the private section, while the other members are protected, but I don't think that it needs to be that way. The _executable could probably just be moved together with the rest of the members.
> 
> OTOH, I think the entire class needs some cleanups. Let's leave this for a separate RFE.

Changed.

> src/hotspot/share/memory/virtualspace.cpp line 708:
> 
>> 706:   assert(max_commit_granularity > 0, "Granularity must be non-zero.");
>> 707: 
>> 708:   _nmt_flag = rs.nmt_flag();
> 
> The code seems to be written with blank lines to separate various members that belong together. Please add a blank line after this line.

Fixed and moved.

> src/hotspot/share/memory/virtualspace.hpp line 199:
> 
>> 197:   size_t _upper_alignment;
>> 198: 
>> 199:   MEMFLAGS _nmt_flag;
> 
> The VirtualSpace::initialize functions used to initialize these members in the order that they are specified here. That is now messed up by adding the _nmt_flag at the end here, but in the beginning in the initialize function. I would propose that you move it to after _executable, both here and in the initialize function.

Fixed.

> test/hotspot/gtest/gc/g1/test_freeRegionList.cpp line 53:
> 
>> 51:   size_t bot_size = G1BlockOffsetTable::compute_size(heap.word_size());
>> 52:   HeapWord* bot_data = NEW_C_HEAP_ARRAY(HeapWord, bot_size, mtGC);
>> 53:   ReservedSpace bot_rs(G1BlockOffsetTable::compute_size(heap.word_size()), mtGC);
> 
> mtGC => mtTest?

Done.

> test/hotspot/gtest/gc/z/test_zForwarding.cpp line 103:
> 
>> 101:     _reserved = reserved;
>> 102: 
>> 103:     os::commit_memory((char*)_reserved, ZGranuleSize, !ExecMem /* executable */, mtGC);
> 
> mtGC => mtTest?

Done.

> test/hotspot/gtest/gc/z/test_zForwarding.cpp line 114:
> 
>> 112:     ZGeneration::_young = _old_young;
>> 113:     if (_reserved != nullptr) {
>> 114:       os::uncommit_memory((char*)_reserved, ZGranuleSize, !ExecMem, mtGC);
> 
> mtGC => mtTest?

Done.

> test/hotspot/gtest/memory/test_virtualspace.cpp line 223:
> 
>> 221:         return ReservedSpace(reserve_size_aligned,
>> 222:                              os::vm_allocation_granularity(),
>> 223:                              os::vm_page_size(), mtTest);
> 
> newline before mtTest.

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1566039719
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1566038228
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1566037774
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1566037525
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1566033618
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1566026381
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1566027896
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1566028800
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1566029979
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1566032851
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1566032647
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1566030330