On Wed, 9 Mar 2022 10:11:06 GMT, Thomas Stuefe <[email protected]> wrote:
>> @parttimenerd please never force-push in an active review as it completely
>> destroys the review history and comment context!
>
> Hi @dholmes-ora , @parttimenerd
>
> I'd like to argue again for my proposal from before.
>
> All this is contrary to how we deal with platform dependencies normally.
> Normally, we
> 1) either keep the whole code in platform specific places - including
> callers. If that is possible.
> 2) If there are very few callers in shared places, we keep implementation in
> platform branch and #ifdef the callsites in shared code out to the affected
> platforms
> 3) If there are many callers in shared code or if it looks like it may be
> useful on other platforms too at some point, we usually wrap the logic in a
> platform generic function behind a descriptive name, which we stub out for
> the unaffected platforms.
>
> (3) is very common.
>
> My proposal from before would make it possible to really hide all WX logic
> from shared code:
>
> shared/runtime/os.hpp
> ```
> class os {
> ...
> // Platform specific hook to prepare the current thread for calling generated
> code
> void enable_jit_calls_for_current_thread() NOT_MACOS_AARCH64({})
> // Platform specific hook to clean up the current thread after calling into
> generated code
> void disable_jit_calls_for_current_thread() NOT_MACOS_AARCH64({})
>
> class ThreadEnableJitCallsMark: public StackObj {
> public:
> ThreadEnableJitCallsMark() { enable_jit_calls_for_current_thread(); }
> ~ThreadEnableJitCallsMark() { disable_jit_calls_for_current_thread(); }
> }
>
>
>
> (ThreadEnableJitCallsMark could be optionally spread out into separate
> include)
>
>
> os.bsd_aarch64.cpp
>
> void os::enable_jit_calls_for_current_thread() {
> ... blabla ... pthread_jit_write_protect_np();
> }
>
> void os::disable_jit_calls_for_current_thread() {
> ... blabla ... pthread_jit_write_protect_np();
> }
>
>
>
> Thats very little code. It effectively hides all platform details where they
> belong, away from shared code. In shared code, you use either one of
> `os::(enable|disable)_jit_calls_for_current_thread()` or the companion
> `os::ThreadEnableJitCallsMark`. Advantages would be:
>
> - Call sites in shared code are now easier to mentally parse.
> `os::disable_jit_calls_for_current_thread()` is much clearer than
> `MACOS_AARCH64_ONLY(os::ThreadWX::Enable __wx(os::ThreadWX::Write));`.
> - We don't need MAC_AARCH64_ONLY in shared code
> - We don't need the enums in shared code. Dont need to understand what they
> do, nor the difference between "Write" and "Exec".
>
> Side note, I'd also suggest a different name for the RAII object to something
> like "xxxMark". That is more according to hotspot customs.
>
> ----
>
> Cheers, Thomas
@tstuefe do you have some examples of (3)? I don't like introducing a fake,
seemingly general-purpose, API for something that is very much platform
specific. I do dislike intensely the way the ThreadWX changes pollute shared
code, and as has been said in other reviews of that code, there really should
be a much cleaner/clearer place where these transitions occur - if we can find
it.
-------------
PR: https://git.openjdk.java.net/jdk/pull/7727
