RFR: 8255254: Split os::reserve_memory and os::map_memory_to_file interfaces [v2]

[Thomas Stuefe]

On Mon, 26 Oct 2020 20:16:32 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.
>
> Anton Kozlov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix review findings

Hi Anton,

this is good! My remarks are all minor nits and/or things which can be fixed in another RFE. Pity that we have to multiplex now in VirtualSpace, but that is still better than what we do now.

Cheers, Thomas

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

> 370:   static char* map_memory_to_file_aligned(size_t size, size_t alignment, int fd);
> 371:   static char* map_memory_to_file(char* base, size_t size, int fd);
> 372:   static char* attempt_map_memory_to_file(char* base, size_t size, int fd);

Can we name this attempt_map_memory_to_file **_at** to have equality to "reserve_memory_at" ?

src/hotspot/os/linux/os_linux.cpp line 4207:

> 4205: }
> 4206: 
> 4207: char* os::pd_attempt_map_memory_to_file(char* requested_addr, size_t bytes, int file_desc) {

This is fine, but I am not sure anymore what the point was of first reserving, then replacing the mapping in the first place. Seems to me one could call os::map_memory_to_file() directly, or am I missing something? 

I think this was somehow all related to https://openjdk.java.net/jeps/316 and allocating on NVDIMM.

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

> 327: 
> 328:   if (end_offset > 0) {
> 329:       os::release_memory(extra_base + begin_offset + size, end_offset);

Not your patch, but the name "end_offset" is seriously confusing here...

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

> 333: }
> 334: 
> 335: // Multiple threads can race in this code, and can remap over each other with MAP_FIXED,

I think you could either completely remove the comment - its somewhat obvious - or remove it down to the call of chop_extra_memory.

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

> 3138: // virtual space to get requested alignment, like posix-like os's.
> 3139: // Windows prevents multiple thread from remapping over each other so this loop is thread-safe.
> 3140: char* map_or_reserve_memory_aligned(size_t size, size_t alignment, int file_desc) {

static ?

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

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