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

Thu Apr 11 16:37:43 UTC 2024

On Thu, 11 Apr 2024 16:10:10 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:
> 
>   fixed missing change.

I think it is good that we are fixing this. I've on the verge of doing it myself a couple of times. I've left an initial set of comments below. It's not an exhaustive list, but I think it is a good set to start with.

src/hotspot/os/bsd/gc/x/xPhysicalMemoryBacking_bsd.cpp line 81:

> 79: 
> 80:   // Reserve address space for backing memory
> 81:   _base = (uintptr_t)os::reserve_memory(max_capacity, false, mtJavaHeap);

I think this should be using !MemExec and not a raw 'false' argument.

With that said, I really think it would be best if we actually split os::reserve_memory into two distinct functions:
1) os::reserve_memory(max_capacity, mtJavaHeap) // Non-executable memory
2) os::reserve_executable_memory(max_capacity, mtCode) // Executable

Maybe we could think about that in separate RFE.

src/hotspot/os/bsd/gc/z/zPhysicalMemoryBacking_bsd.cpp line 82:

> 80: 
> 81:   // Reserve address space for backing memory
> 82:   _base = (uintptr_t)os::reserve_memory(max_capacity, false, mtJavaHeap);

Use !MemExec - this goes for all other places in the patch that fills in the `exec` parameter.

src/hotspot/os/linux/os_linux.cpp line 4684:

> 4682:   char* hint = (char*)(os::Linux::initial_thread_stack_bottom() -
> 4683:                        (StackOverflow::stack_guard_zone_size() + page_size));
> 4684:   char* codebuf = os::attempt_reserve_memory_at(hint, page_size, false, mtInternal);

Should these be `mtInternal` or is there a `mtStack` that is more suitable?

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, mtNone);

Why is this (and the other places in this file) using `mtNone`? Shouldn't it at least be using `mtInternal`?

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1414:

> 1412:   assert(SafepointSynchronize::is_at_safepoint(), "safe iteration is only available during safepoints");
> 1413: 
> 1414:   if (!_aux_bitmap_region_special && !os::commit_memory((char*)_aux_bitmap_region.start(), _aux_bitmap_region.byte_size(), false, mtJavaHeap)) {

Why is this `mtJavaHeap` and not `mtGC`?

src/hotspot/share/gc/z/zMarkStackAllocator.cpp line 107:

> 105: 
> 106:     const uintptr_t shrink_start = _end - shrink_size;
> 107:     os::uncommit_memory((char*)shrink_start, shrink_size, mtGC, false /* executable */);

`uncommit_memory` places the order of executable and flags differently to what we have for `commmit_memory_or_exit`. We might want to consider doing something about the order here.

src/hotspot/share/memory/metaspace.cpp line 592:

> 590:     // Fallback: reserve anywhere
> 591:     log_debug(metaspace, map)("Trying anywhere...");
> 592:     result = os::reserve_memory_aligned(size, Metaspace::reserve_alignment(), false, mtMetaspace);

It's unclear to me if some of these `mtMetaspace` should be `mtClass`. This comment applies to other places where we're setting up memory for the compressed class space.

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

> 69: ReservedSpace::ReservedSpace(size_t size,
> 70:                              size_t alignment,
> 71:                              size_t page_size, MEMFLAGS flag,

I think this function was written to have one argument per line. You should probably keep the style.

I'm also unsure why this param is put as the next to last param instead of the last, as we do in many other places.

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

> 364:   ReservedSpace space;
> 365:   space.initialize_members(base, size, alignment, page_size, special, executable);
> 366:   space.set_nmt_flag(flag);

Why is this calling a set_nmt_flag instead of making initialize_member take a flag?

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

> 691:   _special                = false;
> 692:   _executable             = false;
> 693:   _nmt_flag                = mtNone;

Weird indentation.

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

> 43:   bool   _special;
> 44:   int    _fd_for_heap;
> 45:   MEMFLAGS _nmt_flag;

Indentation is now off.

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

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

No need for the inline specifier here.

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

Changes requested by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18745#pullrequestreview-1994644693
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561286933
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561287120
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561291740
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561294869
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561297501
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561300014
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561301420
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561305069
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561305892
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561306136
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561306502
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561307268