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

[Stefan Karlsson]

On Thu, 18 Apr 2024 09:22:34 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:
> 
>   order of params are consistent now. style is corrected.

More comments.

src/hotspot/os/windows/os_windows.cpp line 5068:

> 5066: char* os::pd_map_memory(int fd, const char* file_name, size_t file_offset,
> 5067:                         char *addr, size_t bytes,
> 5068:                         bool read_only,

This should be reverted.

src/hotspot/share/cds/filemap.cpp line 1701:

> 1699:                              AlwaysPreTouch ? false : read_only,
> 1700:                              allow_exec,
> 1701:                              flags);

Revert this change

src/hotspot/share/cds/filemap.cpp line 1729:

> 1727:                               false /* !read_only */,
> 1728:                               r->allow_exec(),
> 1729:                               mtClassShared);

This mixes styles between multiple arguments per line vs one argument per line.

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 217:

> 215:   if (!_heap_region_special) {
> 216:     os::commit_memory_or_exit(sh_rs.base(), _initial_size, heap_alignment, !ExecMem,
> 217:                               "Cannot commit heap memory", mtGC);

The argument order needs to be changed here and below.

src/hotspot/share/runtime/os.cpp line 2185:

> 2183: char* os::map_memory(int fd, const char* file_name, size_t file_offset,
> 2184:                      char *addr, size_t bytes,
> 2185:                      bool read_only, bool allow_exec, MEMFLAGS flags) {

You should probably move back read_only to the line below.

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

> 518:                            bool read_only,
> 519:                            bool allow_exec,
> 520:                            MEMFLAGS flag);

Style inconsistency

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

Changes requested by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18745#pullrequestreview-2008344764
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1570357660
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1570358795
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1570360021
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1570361901
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1570371965
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1570373261