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

Wed Apr 17 13:08:06 UTC 2024

On Mon, 15 Apr 2024 16:11:13 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:
> 
>   alignment in coding style changed.

Here's a new set of comments.

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

> 5108: 
> 5109:     // Record virtual memory allocation
> 5110:     MemTracker::record_virtual_memory_reserve_and_commit((address)addr, bytes, CALLER_PC, flag);

Should this really be called here? The posix version don't call this, so I don't understand why it is called here in the Windows code.

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

> 1695: static char* map_memory(int fd, const char* file_name, size_t file_offset,
> 1696:                         char *addr, size_t bytes, bool read_only,
> 1697:                         bool allow_exec, MEMFLAGS flags) {

It is odd that `map_memory` and `os::map_memory` has different parameter order. I understand that this is done because of default values, but I'd like to suggest that you get rid of these default values and fix the order.

(Side-note: Wouldn't it be better to rename this `map_memory` to something that clearly shows the difference between this function and `os::map_memory`)

src/hotspot/share/classfile/compactHashtable.cpp line 243:

> 241:     quit("Unable to open hashtable dump file", filename);
> 242:   }
> 243:   _base = os::map_memory(_fd, filename, 0, nullptr, _size, mtInternal, true, false);

Isn't this CDS code. Should ths be mtClassShared or something else that indicates that this is CDS code?

src/hotspot/share/nmt/virtualMemoryTracker.cpp line 460:

> 458:   assert(_reserved_regions != nullptr, "Sanity check");
> 459: 
> 460:   ReservedMemoryRegion  rgn(addr, size, flag);

I'm not sure about this. `rgn` is just used to find the memory region we want to uncommit. The flag isn't used in the search, and passing it forces the callers to also pass in the flag.

I understand that this happens after the request to remove the mtNone default value. Is there a way that allows us to skip using mtNone, but still don't have to unnecessarily provide a flag?

Maybe we could create a helper function `ReservedMemoryRegion rgn = ReservedMemoryRegion::create_find_key(addr, size)`, which sets up a ReserveMemoryRegion with mtNone?

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

> 1815: 
> 1816: char* os::reserve_memory(size_t bytes, bool executable, MEMFLAGS flags) {
> 1817:   char* result = pd_reserve_memory(bytes, executable, flags);

Doesn't it look weird that we pass in flags here and then still call MemTracker::record_ below? I think this is an artifact from mixing if we put the NMT calls in shared or in platform dependent code. I understand that you need this for this patch, but I also think there needs to be some RFE to figure out if this can be reworked.

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

> 2185:                      MEMFLAGS flags,
> 2186:                      bool read_only,
> 2187:                      bool allow_exec) {

The function was written with multiple parameters per line here, and then you changed it so that some of the params where placed on individual lines. This should likely be reverted.

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

> 231:                            char *addr, size_t bytes,
> 232:                            MEMFLAGS flag,
> 233:                            bool read_only = false,

Mixes param layout style. (Plus earlier comment that the default values should probably be removed so that MEMFLAGS can be put last).

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

> 469:   // vm_exit_out_of_memory() with the specified mesg.
> 470:   static void   commit_memory_or_exit(char* addr, size_t bytes,
> 471:                                       bool executable, const char* mesg, MEMFLAGS flag);

I think that we should change the parameter order here, so that it is like `commit_memory` and then the extra mesg param goes with the `_or_exit` part (if that makes sense).
Suggestion:

                                      bool executable, MEMFLAGS flag, const char* mesg);

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

> 520:                            MEMFLAGS flag,
> 521:                            bool read_only = false,
> 522:                            bool allow_exec = false);

params layout style.

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

Changes requested by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18745#pullrequestreview-2005841897
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1568730671
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1568735525
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1568722870
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1568775270
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1568802391
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1568808811
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1568810492
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1568726202
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1568812091