RFR: 8255254: Split os::reserve_memory and os::map_memory_to_file interfaces

[Stefan Karlsson]

On Thu, 22 Oct 2020 15:40:19 GMT, Anton Kozlov <akozlov at openjdk.org> wrote:

> Hi,
> 
> Please review a change to extract map_memory_to_file interface out of reserve_memory when the latter takes file descriptor.
> 
> The change should be a pure refactoring without changes in functionality. The only part is disturbing: a comment in original os_posix.cpp:316 seems to refer to else clause and it contradicts to the actual code.

I think this looks good. Just a few minor comments.

src/hotspot/os/posix/os_posix.cpp line 343:

> 341:   //   mmap but it also may System V shared memory which cannot be uncommitted as a whole, so
> 342:   //   chopping off and unmapping excess bits back and front (see below) would not work.
> 343:   char* extra_base = os::reserve_memory(extra_size);

It seems like this belonged to the fd != -1 clause. It previously talked about the problems of using os::reserve_memory, and then used reserve_mmapped_memory instead in the fd != -1 case. It's not obvious to me that this comment belongs here.

src/hotspot/os/posix/os_posix.cpp line 362:

> 360:   // After we have an aligned address, we can replace anonymous mapping with file mapping
> 361:   if (replace_existing_mapping_with_file_mapping(aligned_base, size, file_desc) == NULL) {
> 362:     vm_exit_during_initialization(err_msg("Error in mapping Java heap at the given filesystem directory"));

There shouldn't be a need to use err_msg for plain strings.

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

> 3174: 
> 3175: char* os::reserve_memory_aligned(size_t size, size_t alignment) {
> 3176:   return map_or_reserve_memory_aligned(size, alignment, -1/*file_desc*/);

Could you add a space between -1 and /* ?

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

> 1740:   }
> 1741:   return result;
> 1742: }

It's a bit unfortunate that the two functions behave differently w.r.t. NMT. They have the same name, but only one reports to NMT. I'd personally would like to transition the code base so that all os::<operation>_memory functions report to NMT, and if you don't wan that you'd use pd_ or internal functions. I think there's a pre-existing bug where a call to os::map_memory_to_file misses the NMT reporting. This is a recurring problem in the os:: layer. Since this is already problematic before your changes, we can investigate this as a separate JBS bug.

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

PR: https://git.openjdk.java.net/jdk/pull/812