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

[Afshin Zafari]

On Fri, 19 Apr 2024 06:23:27 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   more improvements in style/alignments/adjustments.
>
> src/hotspot/os/windows/os_windows.cpp line 3401:
> 
>> 3399: // Reserve memory at an arbitrary address, only if that area is
>> 3400: // available (and not reserved for something else).
>> 3401: char* os::pd_attempt_reserve_memory_at(char* addr, size_t bytes, bool exec, MEMFLAGS nmt_flag) {
> 
> Most of the time the MEMFLAGS parameters are called flag but in some places they are called nmt_flag. Could they always be called flag? This might require some minor renames in some files, but I think that would be OK.

`reserve_large_pages_individually` has a local `flags` variable and
`allocate_pages_individually` has a `flags` param in their definitions. So, the `nmt_flag` is used to avoid confusing different flags while reading the code.
Other cases where there would be no ambiguity, the `nmt_flag` is renamed to `flag`.

> src/hotspot/share/cds/filemap.cpp line 1726:
> 
>> 1724:   char *base = os::map_memory(_fd, _full_path, r->file_offset(),                      //file info
>> 1725:                               addr, size,                                             //memory info
>> 1726:                               false /* !read_only */, r->allow_exec(), mtClassShared); // flags
> 
> Your new comments are not aligned properly.
> 
> However, I still don't think you should do this structural change. It makes the code inconsistent with the code below. If you do want to do it, I think you should change the rest of the code as well. But I don't think you should do that in this RFE, so I'd prefer if you just change this to:
> Suggestion:
> 
>   char *base = os::map_memory(_fd, _full_path, r->file_offset(),
>                               addr, size, false /* !read_only */,
>                               r->allow_exec(), mtClassShared);

Done.

> src/hotspot/share/gc/shared/cardTable.cpp line 87:
> 
>> 85:   ReservedSpace heap_rs(_byte_map_size, rs_align, _page_size, mtGC);
>> 86: 
>> 87:     os::trace_page_sizes("Card Table", num_bytes, num_bytes,
> 
> The indention is now messed up here.

Fixed.

> src/hotspot/share/gc/shared/cardTable.cpp line 176:
> 
>> 174:                                 old_committed.word_size() - new_committed.word_size());
>> 175:     bool res = os::uncommit_memory((char*)delta.start(),
>> 176:                                    delta.byte_size(), !ExecMem, mtGCCardSet);
> 
> Suggestion:
> 
>                                    delta.byte_size(),
>                                    !ExecMem,
>                                    mtGCCardSet);

Done.

> src/hotspot/share/memory/virtualspace.hpp line 180:
> 
>> 178:   bool   _executable;
>> 179: 
>> 180:   MEMFLAGS _nmt_flag;
> 
> Don't place the variable against the comment below.
> Suggestion:
> 
>   MEMFLAGS _nmt_flag;

Done.

> src/hotspot/share/memory/virtualspace.hpp line 199:
> 
>> 197:   size_t _upper_alignment;
>> 198: 
>> 199: 
> 
> Stray blankline

Fixed.

> src/hotspot/share/runtime/os.cpp line 2185:
> 
>> 2183: char* os::map_memory(int fd, const char* file_name, size_t file_offset,  // file info
>> 2184:                      char *addr, size_t bytes,                           // memory info
>> 2185:                      bool read_only, bool allow_exec, MEMFLAGS flags) {  // flags
> 
> I find it odd to make this documentation and separation only for this function. Could you remove that for this PR?
> Suggestion:
> 
> char* os::map_memory(int fd, const char* file_name, size_t file_offset,
>                      char *addr, size_t bytes, bool read_only
>                      bool allow_exec, MEMFLAGS flags) {

Done.

> src/hotspot/share/runtime/os.hpp line 518:
> 
>> 516:   static char*  map_memory(int fd, const char* file_name, size_t file_offset, // file info
>> 517:                            char *addr, size_t bytes,                          // memory info
>> 518:                            bool read_only, bool allow_exec, MEMFLAGS flag);   // flags
> 
> Suggestion:
> 
>   static char*  map_memory(int fd, const char* file_name, size_t file_offset,
>                            char *addr, size_t bytes, bool read_only,
>                            bool allow_exec, MEMFLAGS flag);

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1572020983
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1572021204
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1572021411
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1572021706
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1572021948
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1572022104
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1572024667
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1572024863