On Wed, 9 Mar 2022 10:11:06 GMT, Thomas Stuefe <stu...@openjdk.org> 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