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

Fri Apr 12 07:52:52 UTC 2024

On Thu, 11 Apr 2024 21:25:55 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> `MEMFLAGS flag` is used to hold/show the type of the memory regions in NMT. Each call of NMT API requires a search through the list of memory regions.
>> The Hotspot code reserves/commits/uncommits memory regions and later calls explicitly NMT API with a specific memory type (e.g., `mtGC`, `mtJavaHeap`) for that region.  Therefore, there are two search in the list of regions per reserve/commit/uncommit operations, one for the operation and another for setting the type of the region.  
>> When the memory type is passed in during reserve/commit/uncommit operations, NMT can use it and avoid the extra search for setting the memory type.
>> 
>> Tests: tiers1-5 passed on linux-x64, macosx-aarch64 and windows-x64 for debug and non-debug builds.
>
> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review comments applied.

A few more comments.

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.

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.

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.

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.

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.

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.

src/hotspot/share/memory/virtualspace.cpp line 615:

> 613: 
> 614: ReservedHeapSpace::ReservedHeapSpace(size_t size, size_t alignment, size_t page_size, const char* heap_allocation_directory) : ReservedSpace() {
> 615:   set_nmt_flag(mtJavaHeap);

It seems odd that we only initialize the _nmt_flag when `size == 0`. Could this be done after that check? If not, why not?

There's also a call to record_virtual_memory_type further down in the code. Why is that needed? Why isn't it enough to pass in the correct type to the os::reserve_memory call in the initialize function?

src/hotspot/share/memory/virtualspace.cpp line 672:

> 670:                                      size_t rs_align,
> 671:                                      size_t rs_page_size) : ReservedSpace() {
> 672:   set_nmt_flag(mtCode);

Why isn't this a part of the initialize call? This looks like a bug to me. `initialize` will call clear_members, which will undo this setting.

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.

src/hotspot/share/memory/virtualspace.hpp line 72:

> 70: 
> 71:   MEMFLAGS nmt_flag() { return _nmt_flag; }
> 72:   void set_nmt_flag(MEMFLAGS flag) { _nmt_flag = flag; }

I have a feeling that set_nmt_flag should not exist and be replaced by updated initialize functions.

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.

src/hotspot/share/nmt/virtualMemoryTracker.hpp line 307:

> 305: 
> 306:   ReservedMemoryRegion(address base, size_t size, MEMFLAGS flag) :
> 307:     VirtualMemoryRegion(base, size), _stack(NativeCallStack::empty_stack()), _flag(flag) { }

The function above uses mtNone. I find that a bit dubious, but I understand that it is done to be able to write code like this:

    ReservedMemoryRegion* rmr = VirtualMemoryTracker::_reserved_regions->find(ReservedMemoryRegion(addr, size));

Unfortunately, it opens up the door for people to accidentally use that version instead of this new version that you have written. Could we get rid of the version using mtNone somehow?

The same question goes for the version above that, which has a "MEMFLAGS flag = mtNone". (GH doesn't allow me to comment on lines that you haven't changed)

src/hotspot/share/runtime/os.hpp line 511:

> 509:   // and is added to be used for implementation of -XX:AllocateHeapAt
> 510:   static char* map_memory_to_file(size_t size, int fd, MEMFLAGS flag = mtNone);
> 511:   static char* map_memory_to_file_aligned(size_t size, size_t alignment, int fd, MEMFLAGS flag);

There are still a few mtNone usages in this file.

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?

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?

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?

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.

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

Changes requested by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18745#pullrequestreview-1996032947
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562112282
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562114435
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562114730
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562116176
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562126580
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562134024
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562150698
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562152813
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562154150
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562155538
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562158292
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562163590
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562165152
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562165753
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562166089
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562166154
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562166709