RFR: 8341413: Stop including osThread_os.hpp in the middle of the OSThread class [v2]
Stefan Karlsson
stefank at openjdk.org
Thu Oct 3 11:29:55 UTC 2024
> 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)
Stefan Karlsson has updated the pull request incrementally with three additional commits since the last revision:
- Move NONCOPYABLE
- Move VMStructs fields out of the CPU files
- Add comment to the include of the platform specific class
-------------
Changes:
- all: https://git.openjdk.org/jdk/pull/21306/files
- new: https://git.openjdk.org/jdk/pull/21306/files/27a9567c..04260b95
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=21306&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=21306&range=00-01
Stats: 225 lines in 21 files changed: 48 ins; 143 del; 34 mod
Patch: https://git.openjdk.org/jdk/pull/21306.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/21306/head:pull/21306
PR: https://git.openjdk.org/jdk/pull/21306
More information about the graal-dev
mailing list