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

[Thomas Stuefe]

On Fri, 19 Apr 2024 09:49:33 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:
> 
>   removed extra blank line.

Okay, had another look. Good work. Mostly nits now, but a number of mine and @stefank 's remarks were not addressed. You wrote "Fixed" though. Are you sure you committed your last changes?

I still find it regrettable that we are getting rid of default arguments. This makes many of these APIs rather unwieldy. The patch is massive, which will be a heck of a pain for backporters because patches following this patch will be less likely to be clean backports. 

In a perfect world, one would not call the "raw" os::xxx functions so much and rather use something like ReservedSpace, which then can carry information about the mapping, e.g. the flag. But I never liked ReservedSpace; it feels like old C++ code and often shows surprising behavior. I would love it if someone were to improve that. For example, rewrite initialization to make it more conform to modern C++, and maybe to make it mostly immutable (e.g. Do we really need something like clear_members? Most of the places using clear_members looked to me like they should rely on automatic destructors instead). ReservedSpace is also copied by value, without having a clear semantic of ownership of the underlying mapping.

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

> 611:     }
> 612:   }
> 613: }

stray

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

Changes requested by stuefe (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18745#pullrequestreview-2030234166
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1584156869