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

[Afshin Zafari]

On Wed, 17 Apr 2024 12:02:50 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   alignment in coding style changed.
>
> 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.

It uses the requested address rather than the result of allocation that seems a bug.
Removed.

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

`map_memory` is renamed to `map_and_pretouch_memory`.
argument orders match with `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?

Fixed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1570270206
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1570272820
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1570273489