RFR: 8341413: Stop including osThread_os.hpp in the middle of the OSThread class

[David Holmes]

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)

I personally don't have an issue with the current technique to generate a single platform-specific `OSThread` class, but this refactoring is also okay. There is some unfortunate duplication of the boiler-plate code for the class but not too bad.

I'm tempted to suggest pushing the `_startThread_lock` support into `OSThreadBase` under `#ifndef windows`, just to reduce some duplication. (I may also look at using that for Windows too in the near future, which would address it then.)

I could not see where `thread_type()` is actually used so possibly an additional cleanup opportunity there (not necessarily for this PR).

I don't have any concerns with any of the items that you flagged.

Thanks

src/hotspot/share/runtime/osThread.hpp line 29:

> 27: 
> 28: #include "utilities/macros.hpp"
> 29: #include OS_HEADER(osThread)

Suggestion:


// The actual class declaration is platform specific.
#include OS_HEADER(osThread)

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

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21306#pullrequestreview-2344481343
PR Review Comment: https://git.openjdk.org/jdk/pull/21306#discussion_r1785513385