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.

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(), mtMetaspace);

I would make this mtTest. This should not increase the metaspace counters in NMT

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

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

Not sure if I suggested something different in my first review, but thinking this over, this is wrong. Please don't hardwire mtMetaspace; take the flag from the ReservedSpace member of VirtualSpaceNode.

The reason is that metaspace can be used for at least two different flags, and may later be expanded for more.

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

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

Same here.

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

> 260:     vm_exit_out_of_memory(word_size * BytesPerWord, OOM_MMAP_ERROR, "Failed to reserve memory for metaspace");
> 261:   }
> 262:   MemTracker::record_virtual_memory_type(rs.base(), mtMetaspace);

Looking at this, I don't particularly like it, but it is pre-existing. The fact that we hard-wire mtMetaspace works now relies on the fact that mtClass and mtMetaspace (as of now, the only two flags that are being used) are using different allocation paths. Long term we should change this.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1575752104
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1575747557
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1575747677
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1575763725