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

[Thomas Stuefe]

On Tue, 6 Oct 2020 15:23:56 GMT, Anton Kozlov <akozlov at openjdk.org> wrote:

>>> @tstuefe My patch to remove MAP_FIXED from the memory reservation path should make it possible to revert all the
>>> os::reserve_memory changes in this patch.
>> 
>> FWIW, I now understand that the motivation for the changes in os::reserve_memory is *not* that *it* uses MAP_FIXED.
>> Instead the change there is done so that os::commit_memory doesn't have to mix mmap + MAP_FIXED + MAP_JIT. This is also
>> the reason why os::uncommit_memory needs to be changed as well. So, ignore my comment above.
>
> Hi @tstuefe,
> 
> Recent refactors interfered with the previous version of the patch, I found it is a bit simpler to start from scratch.
> https://github.com/openjdk/jdk/pull/294/commits/f8664ca7dcc1cfdfb9a1f032035f2cde77048649 is a minimal patch that allows
> MAP_JIT. I cannot see any way to simplify the interface now. Please tell me if I miss something. Also, `VirtualSpace`
> now choose from `reserve_memory_with_fd` and anything that is used for reserve with exec.   What I found in the AIX
> implementation, the commit/uncommit are for memory previously reserved
> https://github.com/openjdk/jdk/blob/master/src/hotspot/os/aix/os_aix.cpp#L2227. If we attribute reserved region with
> exec, then uncommit can query bookkeeping info and get the type of memory.  From options of internal bookkeeping info
> or an asking the system for the mmap flags, I prefer the first one. It is faster and we can bookkeep only minimal info,
> like a list of executable regions. That should reduce the amount of data and make a check for the type in
> commit/uncommit faster.  After looking around, it does look like every place where commit with exec is used, we know
> enough to use reserve/uncommit with exec. Taking this to the extreme, I still think a specialized set of
> reserve/commit/uncommit for executable regions would look natural. For example, commit with exec is used in less than
> five places. I'll do a little research there.

> Hi @tstuefe,
> 
> Recent refactors interfered with the previous version of the patch, I found it is a bit simpler to start from scratch.
> [f8664ca](https://github.com/openjdk/jdk/commit/f8664ca7dcc1cfdfb9a1f032035f2cde77048649) is a minimal patch that
> allows MAP_JIT. I cannot see any way to simplify the interface now. Please tell me if I miss something. Also,
> `VirtualSpace` now choose from `reserve_memory_with_fd` and anything that is used for reserve with exec.  What I found
> in the AIX implementation, the commit/uncommit are for memory previously reserved
> https://github.com/openjdk/jdk/blob/master/src/hotspot/os/aix/os_aix.cpp#L2227. If we attribute reserved region with
> exec, then uncommit can query bookkeeping info and get the type of memory.

On AIX there is no explicit commit, committing happens on touch and uncommitting unfortunately does not really work
(there is a disclaim() but its buggy). All commit does is to check the input parameters and maybe explictly touch the
area.

> 
> From options of internal bookkeeping info or an asking the system for the mmap flags, I prefer the first one. It is
> faster and we can bookkeep only minimal info, like a list of executable regions. That should reduce the amount of data
> and make a check for the type in commit/uncommit faster.  After looking around, it does look like every place where
> commit with exec is used, we know enough to use reserve/uncommit with exec. Taking this to the extreme, I still think a
> specialized set of reserve/commit/uncommit for executable regions would look natural. For example, commit with exec is
> used in less than five places. I'll do a little research there.

I thought a lot about this.

As you said, I believe now that specifying exec (or other parameters, see below) is not necessary on a per-commit; on
per-mapping level it should be enough.

I also see at least three separate cases where we establish a mapping and later need mapping-specific information
somewhere until the next interaction - be it commit/uncommit or release:

1) On AIX, where we decide at os::reserve_memory time to use either SystemV or mmap and later need to know
2) On Linux when TPH are active the information "Use TPH" is handed down to os::commit via the weird alignment_hint
parameter and that os::realign_memory() function. Following that parameter flow, it is just basically a way to flag the
code to do one extra madvice at commit time. 3) Your case now, where we need to know if the mapping was supposed to be
executable at commit time.

I really think a generic solution would be best. One simple variant would be to return not a pointer but a handle and
let the platform store behind that handle whatever it needs to keep on a per-mapping base:

handle_t os::reserve_memory(size, input flags);
void* start_address = os::get_reserved_memory_start_address(handle_t)
bool os::commit_memory(handle_t, address, size)

and so on. The only problem with that is that it would cause a lot of call sites to change, and the callers need to
hold on to that mapping handle.

To be clear, I do not think you should do this with this patch, but I would like your opinion, since you looked at the
code closely.

I'll review the current version presently.

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

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