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

[Thomas Stuefe]

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.

Mostly good. Small nits inside.

src/hotspot/share/memory/metaspace/testHelpers.cpp line 81:

> 79:   if (reserve_limit > 0) {
> 80:     // have reserve limit -> non-expandable context
> 81:     _rs = ReservedSpace(reserve_limit * BytesPerWord, Metaspace::reserve_alignment(), os::vm_page_size(), mtTest);

mtMetaspace

src/hotspot/share/memory/metaspace/virtualSpaceNode.cpp line 112:

> 110: 
> 111:   // Commit...
> 112:   if (os::commit_memory((char*)p, word_size * BytesPerWord, !ExecMem, _rs.nmt_flag()) == false) {

just use mtMetaspace here, its easier

src/hotspot/share/memory/metaspace/virtualSpaceNode.cpp line 191:

> 189: 
> 190:   // Uncommit...
> 191:   if (os::uncommit_memory((char*)p, word_size * BytesPerWord, !ExecMem, _rs.nmt_flag()) == false) {

mtMetaspace

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

> 519:                            bool allow_exec = false, MEMFLAGS flags = mtNone);
> 520:   static bool   unmap_memory(char *addr, size_t bytes);
> 521:   static void   free_memory(char *addr, size_t bytes, size_t alignment_hint, MEMFLAGS flag);

While looking at this, I noticed a couple of odd things about this function. I think it should be revised and I opened https://bugs.openjdk.org/browse/JDK-8330144. The result of that revision will be that we don't need MEMFLAGS, nor do would we need the alignment hint. 

But leave the MEMFLAGS in for now. If I happen to push that change first, you can adapt the change, if you push first I'll manage.

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

PR Review: https://git.openjdk.org/jdk/pull/18745#pullrequestreview-1996103064
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562159668
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562159155
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562159372
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1562178816