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

[Anton Kozlov]

On Sat, 12 Dec 2020 09:29:08 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 style
>
> Hi Anton,
> 
> 1) can you please make the executable parameter on os::uncommit() default false? Lets minimize the impact for the vast majority of callers which do not need protected memory. The only caller needing this is ReservedSpace. Would also be more in sync with the default false for executable on os::reserve(). 
> 
> 2) Personal nit, I really find this `ExecMem` jarring. We don't do this (pass named aliases for boolean flags) for any other arguments AFAICS. The usual way to emphasize arg names is with comments:
> bool result = os::commit_memory(base, size, /*exec*/ false);
> If you still prefer to use it, could you leave at least those places unchanged which are unaffected by your patch?
> 
> 3) 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?
> 
> 4) 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.
> 
> Thanks, Thomas

Hi Thomas, 

Thank you for review!

> 1. can you please make the executable parameter on os::uncommit() default false? 

Ok, fixed.

> 2. 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)

> 3. 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.

> 2. 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. 

Thanks,
Anton

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

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