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

[Anton Kozlov]

On Tue, 15 Dec 2020 16:06:23 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Anton Kozlov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Update pd_commit_memory on bsd
>
>> Hi Thomas,
>> 
>> Thank you for review!
>> 
>> > 1. can you please make the executable parameter on os::uncommit() default false?
>> 
>> Ok, fixed.
> 
> Thanks!
> 
>> 
>> > 1. Personal nit, I really find this `ExecMem` jarring.
>> >    If you still prefer to use it, could you leave at least those places unchanged which are unaffected by your patch?
>> 
>> The only two places where I had to replace false with !ExecMem are https://github.com/openjdk/jdk/pull/294/files#diff-80d6a105c4da7337cbc8c4602c8a1582c6a1beb771797e1a84928bd864afe563R105
>> 
>> It would be really inconsistent to maintain them as
>> 
>> ```
>> char* base = os::reserve_memory(size, !ExecMem, mtThreadStack);
>> bool result = os::commit_memory(base, size, false);
>> ```
>> 
>> (Please note that the false without comment didn't obey any style, so I had to fix these anyway)
> 
> Okay.
> 
>> 
>> > 1. There is some code explicitly dealing with the supposed inability of attempt_reserve_memory_at() using MAP_JIT. I don't understand. I thought the problem was thjat MAP_JIT and MAP_FIXED don't mix. But attempt_reserve_memory_at() does explicitely not use MAP_FIXED, it just attempts to map at a non-null wish address. Does that also not work with MAP_JIT?
>> 
>> Interesting and funny, it does work. I would expected it does not (due to security reasons I could image behind forbidding MAP_JIT|MAP_FIXED). But since it works for now, I've added the executable parameter to attemp_reserve_memory_at as well.
> 
> I am not that surprised. Wish address != NULL with MAP_FIXED=0 just establishes a brand new mapping, does not change an existing mapping. I think they just forbid to modify existing mappings with MAP_JIT once established.
> 
>> 
>> > 1. For my taste there are too many unrelated changes, especially in ReservedSpace. Making all those file scope static helpers members of ReservedSpace causes a lot of diffs. Makes sense cleanup-wise but will make backporting your patch more difficult later (I expect this will be a strong candidate for backporting). Please tone down the patch a bit. I pointed some parts out directly below. Beyond those, I leave it up to you how far you minimize the patch.
>> 
>> Ok, it is possible to do the clean up after. Thanks for your comment, I'll account them in the future clean-up RFR.
>> 
> 
> Great, thanks, the change looks much cleaner now.
> 
>> Thanks,
>> Anton
> 
> There are two remaining very minor nits, I leave it up to you if you fix them. From my eyes this is fine. 
> 
> -----
> 
> I think we should shake up that coding at some point and improve the API. I imagine something along the line of
> 
> struct mappinginfo_t { 
>    pagesize, // may be dynamically chosen by the OS layer but caller wants to know
>    exec, 
>    base, size, ... // maybe
>    + maybe opaque OS specific information
> };
> address os::reserve_xxxx(size, ..., reservation_info_t* p_info = NULL);
> bool os::commit_memory(addr, size, const reservation_info_t* info);
> bool os::commit_memory(addr, size, const reservation_info_t* info);
> 
> The concrete form can be shaped however, but the base idea is to return more information than just the reservation pointer:
> - some attributes may be chosen or adapted by os::reserve_... (eg PageSize), but caller could just be told to spare him having to second-guess  os::reserve_memory_special().
> - some attributes may be only interesting to the os layer, so they can be opaque for the caller - but caller could hold onto that information until commit/uncommit. Examples for that are AIX: mmap-or-shmat , or Windows: NUMA-striped-allocation or not.
> - some information may be caller specified (eg exec) but returning this in a handle-like structure relieves the caller from holding on to that particular information, and having to pass each argument separately-
> 
> I know this is crossing territory into what ReservedSpace does today, but that class is quite polluted with VM specific stuff, eg that noaccess zone. Well, lets see how this goes.
> 
> Thanks alot for your perseverance!
> 
> Cheers, Thomas

Thomas, thank you very much for all the comments, insights, and all of your time. I think a have a good queue of future enhancements from various approaches we've discussed and tried, small and big ones :)

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

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