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

[Afshin Zafari]

On Thu, 23 May 2024 12:45:37 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fixed the missing parts of shenandoahHeap.cpp
>
> src/hotspot/share/memory/virtualspace.hpp line 76:
> 
>> 74: 
>> 75:   MEMFLAGS nmt_flag() const { assert(is_reserved(), "Memory region is not reserved."); assert(_flag != mtNone, "Memory flag is not set."); return _flag; }
>> 76: 
> 
> Looking at this again, and realize that this function should probably be moved to the other accessors below.

Done.

> src/hotspot/share/memory/virtualspace.hpp line 98:
> 
>> 96:   bool   special()         const { assert(is_reserved(), "Memory region is not reserved."); return _special;   }
>> 97:   bool   executable()      const { assert(is_reserved(), "Memory region is not reserved."); return _executable;   }
>> 98:   size_t noaccess_prefix() const { assert(is_reserved(), "Memory region is not reserved."); return _noaccess_prefix;   }
> 
> FWIW, this change comes from one of my debugging sessions. I think it is good to have these asserts, I just wish they could says something like `assert(is_initialized(), ...)` to more clearly convey why we are doing this check.
> 
> We are considering if there are ways to split ReservedSpace into two classes, one that handles reserving of memory and one that is a plain view of already reserved memory. If/when we do such a change we could consider updating these asserts to be more legible.
> 
> In the meantime, it would be nice to change the string to "Fields not initialized" (and get rid of the `.`).

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19343#discussion_r1613185089
PR Review Comment: https://git.openjdk.org/jdk/pull/19343#discussion_r1613185323