RFR: 8234930: Use MAP_JIT when allocating pages for code cache on macOS [v6]

[Anton Kozlov]

On Thu, 3 Dec 2020 15:19:51 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>>> Do you think os::protect_memory(base, size, os::MEM_PROT_RWX) will work on all platforms?
>> 
>> It looks so. Not sure about AIX, it ends up with ::mprotect, at least theoretically should be fine. On Linux and macOS without hardening it should be also OK. Without this patch, we don't support macOS hardened mode at all. But I have some private hacks for CDS with hardening, they reuse some of windows code for reading the archive content instead of mapping, so the future looks even more convenient.
>
> Hi Anton,
> 
> Unfortunately I am not sure anymore that a separate API for reserving code is practical. See https://github.com/openjdk/jdk/pull/1153 (https://bugs.openjdk.java.net/browse/JDK-8256155). People want to be able to use large paged memory for code. Large paged memory gets allocated via os::reserve_memory_special(). 
> 
> Today we already split the API space into two groups: os::reserve_memory() and friends, and os::(reserve|release)_memory_special(). Adding an "executable" API group to that would multiply the number of APIs by two. I am afraid we are stuck with the exec flag on reserve and commit.
> 
> If you are interested, there was a lively discussion under https://github.com/openjdk/jdk/pull/1161 (https://bugs.openjdk.java.net/browse/JDK-8243315). Among other things it was discussed whether we should get rid of multi-page regions (mixing various page sizes). See Linux::reserve_memory_special_hugetlb_mixed. This would simplify coding.
> 
> Since I feel bad now for causing you work, I give up any opposition to extending the APIs with the exec parameter. So I had a closer look at your original change again:
> 
> https://github.com/openjdk/jdk/pull/294/commits/114d9cffd62cab42790b65091648fe75345c4533
> 
> I wonder whether we could simplify things, if we let go of the notion that the code heap gets only committed on demand. I do not know how MacOS memory overcommit works in detail. But on Linux, committing memory increases process footprint toward the commit charge limit, and may need swap space, but it does not increase RSS as long as the memory is not touched. I do not know how important on MacOS delaying memory commit really is. If its not important, then we could just do this:
> 
> reserve_memory :
> 	- not executable: mmap MAP_NORESERVE, PROT_NONE
> 	- executable: mmap MAP_JIT *without* MAP_NORESERVE, PROT_READ|PROT_WRITE|PROT_EXEC (so its committed and accessible right away)
> 
> commit_memory
> 	- not executable: mmap without MAP_NORESERVE, PROT_READ|PROT_WRITE
> 	- executable: (return, nothing to do)                   
> 
> uncommit_memory
> 	- not executable: mmap MAP_NORESERVE, PROT_NONE
> 	- executable: (return, nothing to do, since you indicate that this is that memory does not get returned to the OS immediately)
> 
> Furthermore, about uncommit: I wonder whether madvice(MADV_FREE) would alone be already sufficient to release the memory. I have no Mac and cannot test this. The range would still be accessible though, but combining that with mprotect(PROT_NONE) should take care of this. Then we could just in general avoid the mmap(MAP_NORESERVE|MAP_FIXED) call. Then we do not need the exec parameter for uncommit at all.
> 
>> Two functions like reserve_memory and reserve_memory_aligned look excessive. ReservedSpace for some reason tries to use the unaligned version first and when it fails (how should it know the result should be aligned?), fallbacks to reserve_memory_alignment. https://github.com/openjdk/jdk/blob/master/src/hotspot/share/memory/virtualspace.cpp#L227. It will be more straightforward to ask for alignment from the start when it's required. I'm going to make alignment a parameter for reserve_memory as well, with the default value to be "no specific alignment", and to remove reserve_memory_aligned. It will simplify the implementation of reserve_executable_memory with alignment argument, and I hope to propose the suggested refactoring separately from this PR.
>> 
> 
> This makes sense, but is outside the scope of this RFE.
> 
> In general, I think in the API we need a separation between page size and alignment (both have been confused in the past, see discussion under https://github.com/openjdk/jdk/pull/1161). But page size is irrelevant for reserve_memory - which reserves per default just with os::vm_page_size() - but for reserve_memory_special we should specify both.
> 
> Cheers, Thomas

> Unfortunately I am not sure anymore that a separate API for reserving code is practical. See #1153 (https://bugs.openjdk.java.net/browse/JDK-8256155). People want to be able to use large paged memory for code. Large paged memory gets allocated via os::reserve_memory_special().

I think os::reserve_memory_special is substantially different, it does not allow commit/uncommit. https://github.com/openjdk/jdk/blob/master/src/hotspot/share/memory/virtualspace.cpp#L170 basically defines it this way. I'm mostly concerned with interface for executable memory with commit.

> Today we already split the API space into two groups: os::reserve_memory() and friends, and os::(reserve|release)_memory_special(). Adding an "executable" API group to that would multiply the number of APIs by two. I am afraid we are stuck with the exec flag on reserve and commit.

It adds another API. And it's interesting that the only user of executable_memory are ReservedSpace and VirtualSpace. Removing executable argument from the rest of cases seems beneficial. I find it in somewhat more strict comparing with a boolean flag. But it's not really required for MAP_JIT.

>From interface implementation point of view, it's convenient to have a single interface with as many parameters as possible, even excessive and unused. It will enable tricky cases handling inside the implementation. executable_memory API is artificial separation in this case. It will be necessary if some combination of parameters are impossible to implement, but it's not our case, so we can live without it.

> I wonder whether we could simplify things, if we let go of the notion that the code heap gets only committed on demand. 

I'm not sure, what is the aim of the simplification below? Now access to uncommitted memory will cause a trap, just like on other platforms.

> I do not know how MacOS memory overcommit works in detail. But on Linux, committing memory increases process footprint toward the commit charge limit, and may need swap space, but it does not increase RSS as long as the memory is not touched. I do not know how important on MacOS delaying memory commit really is. If its not important, then we could just do this:
> 
> reserve_memory :
> - not executable: mmap MAP_NORESERVE, PROT_NONE
> - executable: mmap MAP_JIT _without_ MAP_NORESERVE, PROT_READ|PROT_WRITE|PROT_EXEC (so its committed and accessible right away)
> 
> commit_memory
> - not executable: mmap without MAP_NORESERVE, PROT_READ|PROT_WRITE
> - executable: (return, nothing to do)
> 
> uncommit_memory
> - not executable: mmap MAP_NORESERVE, PROT_NONE
> - executable: (return, nothing to do, since you indicate that this is that memory does not get returned to the OS immediately)
> 

> Furthermore, about uncommit: I wonder whether madvice(MADV_FREE) would alone be already sufficient to release the memory. I have no Mac and cannot test this. The range would still be accessible though, but combining that with mprotect(PROT_NONE) should take care of this. Then we could just in general avoid the mmap(MAP_NORESERVE|MAP_FIXED) call. Then we do not need the exec parameter for uncommit at all.

madvise(FREE) is not sufficient unfortunately. For executable memory, it's best we can do. But we should not use it for regular memory.

> In general, I think in the API we need a separation between page size and alignment (both have been confused in the past, see discussion under #1161). But page size is irrelevant for reserve_memory - which reserves per default just with os::vm_page_size() - but for reserve_memory_special we should specify both.

If we talk about reveting to 114d9cf, there is no change beyond extra boolean argument, right?

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

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