RFR: 8341413: Stop including osThread_os.hpp in the middle of the OSThread class
Axel Boldt-Christmas
aboldtch at openjdk.org
Thu Oct 3 05:54:37 UTC 2024
On Wed, 2 Oct 2024 13:50:01 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:
> Some HotSpot files have an interesting include pattern where the platform dependent code is included straight into the class containing the shared implementation.
>
> This has various interesting effects to the code layout and readability of the code. I propose we stop doing this, where possible, and instead clearly separate the shared code and the platform specific code in separate classes. This then allows us to use the standard include patterns that we use for most of our code.
>
> This PR is a suggestion for how to untangle this for the OSThread class.
>
> Things in the code that changed with this patch that might be good to take an extra look at:
> 1) I dropped unnecessary includes
> 2) `pd_initialize/pd_destroy` was converted into constructor/destructor
> 3) Member initialization lists are used. Note that some variables that used to be uninitialized are now getting initialized. `_caller_sigmask` is one of the interesting once, because it's sizable array. I still don't think that is problematic because all the other code we run to start threads, but if I get push back on this I'll comment it out.
> 4) (3) switched the order of the `new Monitor` call and the `sigemptyset` call
> 5) I did some reordering of functions to unify the four platforms
> 6) Moved `_thread_id` to the platform files
> 7) I stopped exposing the `thread_id_t` typedef.
> 8) I created a virtual `thread_id_for_printing` function for those calls that want a unified integer type that can be printed. An alternative to this could be to use a non-virtual call, but that requires us to to define `OSThreadBase::thread_id_for_printing` in the platform files.
>
> Tested: tier1-3, (excluding AIX, which I can't build/test)
It is interesting to me that we are using `AllocFailStrategy::RETURN_NULL` when allocating the `OSThread` but then do a `AllocFailStrategy::EXIT_OOM` allocation of a Monitor in the middle of constructing the `OSThread`.
Even though I am not sure about the usefulness of `AllocFailStrategy::RETURN_NULL`, and whether if it is ever recoverable to fail (smallish) native allocations in Hotspot. Seems like we will crash very soon in any case.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/21306#issuecomment-2390579652
More information about the hotspot-dev
mailing list