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

[Afshin Zafari]

On Tue, 23 Apr 2024 07:05:24 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> 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

Done.

> 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.

Done.

> 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.

Done.

> 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.

Should I create a RFE for it?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1575896068
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1575895625
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1575895817
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1575899343