[jdk11u-dev] RFR: 8234930: Use MAP_JIT when allocating pages for code cache on macOS

[Thomas Stuefe]

On Wed, 8 Dec 2021 16:55:16 GMT, Vladimir Kempik <vkempik at openjdk.org> wrote:

> Hello, here is alternative backport of 8234930 to jdk11u.
> Very minimalistic change for platforms other than macos, just one unused argument.
> We have used this patch in zulu11 for more than a year now and found no issues with it on macos and other platforms.
>
> Technically it's mostly the same as previous (3patch) version but has no prerequests.

Hi Vladimir,

this is already a lot better.

And slowly the whole story comes back to me. IIRC the problem is that MAP_JIT cannot be used in conjunction with MAP_FIXED, right? Therefore we cannot replace mappings? Therefore our traditional technique of doing commit and uncommit - replacing a mapping with another one at the same address range but swapped MAP_NORESERVE - does not work?

If this is true, could you please add a comment somewhere at the start of the file explaining this, and refer in both uncommit and commit to that?

Additionally, also modify the associated JBS issue text and add it there too? Feel free to use the same text, I just don't want that knowledge to be gone. I remember Anton had a hard time explaining things to me last year, so it's not that obvious.

---

Please also add a comment to os_bsd.cpp to state that the "exec" flag basically means "its code cache" and that it should be used consistently for the same mapping (reserve-commit-uncommit etc).

---

The change can be made a lot smaller still. Sure we will divert from upstream over time, but keeping patches small helps delay that.

1) Please make the exec parameter to commit() and uncommit() default false and remove all invocations wich use false or !ExecMem explicitly. That is the lion's share of calls.
2) Please, instead of adding "exec" to every version of pd_commit/uncommit, only add it for MacOS like this:


pd_uncommit_memory(char* addr, size_t bytes MACOS_ONLY(, bool exec));
pd_commit_memory(char* addr, size_t bytes MACOS_ONLY(, bool exec));


and do the same with the two places where this parameter actually matters on invocation (os.cpp and VirtualSpace).

---

Thanks, Thomas

src/hotspot/os/bsd/os_bsd.cpp line 2010:

> 2008: bool os::pd_commit_memory(char* addr, size_t size, bool exec) {
> 2009:   int prot = exec ? PROT_READ|PROT_WRITE|PROT_EXEC : PROT_READ|PROT_WRITE;
> 2010: #if defined(__OpenBSD__)

Unnecessary change, lets keep the patch minimal

src/hotspot/os/bsd/os_bsd.cpp line 2016:

> 2014:     return true;
> 2015:   }
> 2016: #elif defined(__APPLE__)

I'm confused here. We reserve with MAP_NORESERVE, then, on commit, just mprotect? Will that remove the MAP_NORESERVE flag?

Why not do mprotect only if exec=true (which we treat here synonymous for "is code cache area")?

src/hotspot/share/runtime/os.hpp line 331:

> 329:   static char*  reserve_memory(size_t bytes, char* addr = 0,
> 330:                                size_t alignment_hint = 0, int file_desc = -1,
> 331:                                bool executable = false);

Please make this default=false too, and revert the hunks where this is explicitly set to false

src/hotspot/share/runtime/os.hpp line 348:

> 346:                                       size_t alignment_hint,
> 347:                                       bool executable, const char* mesg);
> 348:   static bool   uncommit_memory(char* addr, size_t bytes, bool exec);

Please make this default=false too, and revert the hunks where this is explicitly set to false

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

Changes requested by stuefe (Reviewer).

PR: https://git.openjdk.java.net/jdk11u-dev/pull/710