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

Wed Dec 2 21:14:57 UTC 2020

On Wed, 14 Oct 2020 10:56:25 GMT, Anton Kozlov <akozlov at openjdk.org> wrote:

>>> > GrowableArray maybe not the best choice here since e.g. it requires you to search twice on add. A better solution may be a specialized BST.
>>> 
>>> I assume amount of executable mappings to be small. Depends on if exec parameter available at reserve, it is either only a single one for the CodeCache (see below) or plus several more for mappings with unknown mode (that were not committed yet)
>>> 
>>> > IMHO too heavvy weight for a platform only change.
>>> > If there are other uses for such a solution (managing memory regions, melting them together, splitting them maybe on remove)
>>> > we should not support setting and clearing exec on commit but only on a per-mapping base.
>>> 
>>> It is more simple when the whole mapping is executable or not. We don't need to split/merge on commit/uncommit then. But we need do to something when os::release_memory is called on a submapping of a mapping with unknown status. Like on AIX, uncommit is made https://github.com/openjdk/jdk/blob/master/src/hotspot/os/aix/os_aix.cpp#L2096. But here for macOS, I'm trying to avoid any change of behavior for non-exec mappings.
>>> 
>>> If the exec parameter is provided for reserve (as it eventually would be), then we don't need splitting/merging at all. This is what the latest patch is about. I haven't tested that thoroughly yet, but eventually it would be possible to deduce correct exec values for os::reserve based on subsequent os::commit. If we make a step back, we have exec parameter known for reserve and commit, I also pretty sure that it is possible to deduce that for any uncommit (which was one of the initial concerns)
>>> 
>>> Let's agree on some plan how to attack the problem? I would like to distinguish the work toward MAP_JIT and improving interface. Not sure what should come first. Are you still opposing to have exec parameter in os::reserve/commit/uncommit and obligating callers to provide consistent exec values for each, at least at this phase?
>>> 
>>> I mean, eventually we will have a platform-dependent `handle_t` for mapping or equivalent. Like if we provide size of the whole mapping (the context) for each commit_memory on AIX, we won't need to do the bookkeeping. What if os::commit to take ReservedSpace and do something conservative when that is not provided?
>> 
>> 
>> 
>>> > GrowableArray maybe not the best choice here since e.g. it requires you to search twice on add. A better solution may be a specialized BST.
>>> 
>>> I assume amount of executable mappings to be small. Depends on if exec parameter available at reserve, it is either only a single one for the CodeCache (see below) or plus several more for mappings with unknown mode (that were not committed yet)
>>> 
>>> > IMHO too heavvy weight for a platform only change.
>>> > If there are other uses for such a solution (managing memory regions, melting them together, splitting them maybe on remove)
>>> > we should not support setting and clearing exec on commit but only on a per-mapping base.
>>> 
>>> It is more simple when the whole mapping is executable or not. We don't need to split/merge on commit/uncommit then. But we need do to something when os::release_memory is called on a submapping of a mapping with unknown status. Like on AIX, uncommit is made https://github.com/openjdk/jdk/blob/master/src/hotspot/os/aix/os_aix.cpp#L2096. But here for macOS, I'm trying to avoid any change of behavior for non-exec mappings.
>>> 
>>> If the exec parameter is provided for reserve (as it eventually would be), then we don't need splitting/merging at all. This is what the latest patch is about. I haven't tested that thoroughly yet, but eventually it would be possible to deduce correct exec values for os::reserve based on subsequent os::commit. If we make a step back, we have exec parameter known for reserve and commit, I also pretty sure that it is possible to deduce that for any uncommit (which was one of the initial concerns)
>>> 
>>> Let's agree on some plan how to attack the problem? I would like to distinguish the work toward MAP_JIT and improving interface. Not sure what should come first. Are you still opposing to have exec parameter in os::reserve/commit/uncommit and obligating callers to provide consistent exec values for each, at least at this phase?
>>> 
>>> I mean, eventually we will have a platform-dependent `handle_t` for mapping or equivalent. Like if we provide size of the whole mapping (the context) for each commit_memory on AIX, we won't need to do the bookkeeping. What if os::commit to take ReservedSpace and do something conservative when that is not provided?
>> 
>> Are there any users of executable memory which cannot live with anonymous mapping on whatever address with small pages? Does anyone need large pages or a specific wish address?
>> 
>> If not, maybe we really should introduce a (reserve|commit|uncommit|release)_executable_memory() at least temporarily, as you suggested. At least that would be clear, and could provide a clear starting point for a new interface.
>
>> Are there any users of executable memory which cannot live with anonymous mapping on whatever address with small pages? Does anyone need large pages or a specific wish address?
> 
> Nothing jumps out immediately. 
> Recently we've come across CDS problems, which also requires executable permissions, but it uses file-based mapping and os::map_memory.
> 
>> If not, maybe we really should introduce a (reserve|commit|uncommit|release)_executable_memory() at least temporarily, as you suggested. At least that would be clear, and could provide a clear starting point for a new interface.
> 
> Then I'll start doing this. I'll create another JBS issue for the interface closer to the point when it is ready.
> 
> Thanks!

Hi, I've just pushed an update with the new executable_memory interface. Still WIP, few notes and one major problem description follow. 

The new interface does not allow reserve executable memory with a specific address (restriction of MAP_JIT). Before the change, such executable memory was required by
* workaround on 32bit linux-x86 https://github.com/openjdk/jdk/pull/294/files#diff-ec5e71a69afd99d4cfec5f5c657242bae9434656025694e67cae03a5e3722e84
* reading of CDS archive on windows https://github.com/openjdk/jdk/pull/294/files#diff-c93e710cbc38c989c0ab250cd4ac04d2ab157f44ee535bb035f473c42d0557c7
In both cases, it was changed to reserve non-executable memory and mprotect, which produce same result as before. Generally speaking, only platform-specific workarounds were requiring a specific address for executable mapping.

MAP_JIT implementation on top of the new interface is really tiny, so it does not need a separate commit, I think.

The major problem here is that executable memory now cannot be reserved at a certain alignment, although it may be required. I think the alignment should be an extra parameter to executable_memory_reserve and not e.g. reserve_executable_memory_aligned. So I'll work in this direction.

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.

Please let me know if I missed something, or going to miss :)

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

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