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

stefank · 2024-10-02T13:50:01Z

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:

I dropped unnecessary includes
pd_initialize/pd_destroy was converted into constructor/destructor
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.
(3) switched the order of the new Monitor call and the sigemptyset call
I did some reordering of functions to unify the four platforms
Moved _thread_id to the platform files
I stopped exposing the thread_id_t typedef.
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)

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8341413: Stop including osThread_os.hpp in the middle of the OSThread class (Enhancement - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer) Review applies to 27a9567c
Coleen Phillimore (@coleenp - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21306/head:pull/21306
$ git checkout pull/21306

Update a local copy of the PR:
$ git checkout pull/21306
$ git pull https://git.openjdk.org/jdk.git pull/21306/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21306

View PR using the GUI difftool:
$ git pr show -t 21306

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21306.diff

Webrev

Link to Webrev Comment

… class

bridgekeeper · 2024-10-02T13:50:51Z

👋 Welcome back stefank! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-10-02T13:51:43Z

@stefank This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: coleenp, dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 45 new commits pushed to the master branch:

21f8ccf: 8341310: Test TestJcmdWithSideCar.java should skip ACCESS_TMP_VIA_PROC_ROOT (after JDK-8327114)
7d524d7: 8341004: Open source AWT FileDialog related tests
d7f32d8: 8341415: Optimize RawBytecodeHelper::next
6af1358: 8337753: Target class of upcall stub may be unloaded
19642bd: 8341148: Open source several Choice related tests
0b467e9: 8334060: Implementation of Late Barrier Expansion for G1
c6e7e55: 8341091: CDS: Segmented roots array misses roots
ff3e849: 8341239: Open source closed frame tests # 3
50ec169: 8341191: Open source few more AWT FileDialog tests
602408e: 8341177: Opensource few List and a Window test
... and 35 more: https://git.openjdk.org/jdk/compare/1b46fea59cf8f53b23e5c16a604b4decc8c7dbbe...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-10-02T13:52:45Z

@stefank The following labels will be automatically applied to this pull request:

graal
hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-10-02T13:56:09Z

Webrevs

01: Full - Incremental (04260b95)
00: Full (27a9567c)

dholmes-ora

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

dholmes-ora · 2024-10-03T01:43:13Z

Aside: forgot to mention, something that seemed odd to me is why we have the OSThread stuff in the os-cpu VMStructs files instead of the os one? I didn't spot anything CPU specific about that.

xmas92 · 2024-10-03T05:51:51Z

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.

dholmes-ora · 2024-10-03T08:35:40Z

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.

True. The problem with allocations within the constructor is that we have no way to convey to the caller that we had a failure. So if allocation of OSThread fails we act like it is non-fatal because we can just throw a Java exception from Thread.start. But if the constructor fails to construct things properly we have no mechanism to communicate that so we abort on internal allocation failures.

xmas92 · 2024-10-03T09:10:23Z

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.

The problem with allocations within the constructor is that we have no way to convey to the caller that we had a failure.

Setting a bool field during construction is something we do in a lot of places to signal if the construction was successful.

  OSThread* osthread = new (std::nothrow) OSThread();
  if (osthread == nullptr || osthread->has_constructor_failed()) {
    delete osthread;
    return false;
  }

coleenp

Nice cleanup but one question about SA.

coleenp · 2024-10-03T11:42:23Z

src/hotspot/os_cpu/bsd_aarch64/vmStructs_bsd_aarch64.hpp

-  /* Threads (NOTE: incomplete) */                                                                                                   \
-  /******************************/                                                                                                   \
-  nonstatic_field(OSThread,                      _thread_id,                                      OSThread::thread_id_t)             \
-  nonstatic_field(OSThread,                      _unique_thread_id,                               uint64_t)


Does the SA actually use these? I don't see any SA changes in this patch. If it doesn't, we should remove it until there is someone that might add support for this. We're not enhancing the SA at this point.

I think the SA is using these. There are references to them in the various SA files.

Ok. I see now why you don't also have to change SA.

stefank · 2024-10-03T12:56:06Z

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.

Thanks for not blocking this suggestion.

There is some unfortunate duplication of the boiler-plate code for the class but not too bad.

I pushed a change that moved the NONCOPYABLE macro. I did a little experiment with moving even more code to the OSThreadBase, but I'm not sure if this is wanted or not. I've put that change here in-case someone wants to take a look:
pr/21306...stefank:jdk:osThread_follow_up

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'll leave this for next time someone wants to do a little bit of clean-up in this area.

One interesting thing that we found while looking at this is that the AIX port has the _startThread_lock, but it doesn't use it to coordinate the starting of the created thread. It's unclear to me if that's a bug in that port.

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

Maybe one reason to keep it is as an aid when debugging with a debugger? OTOH, while playing around with the patch linked above I found that the field is not available on Windows, and it's also left uninitialized when we call create_attached_thread.

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

Thanks

Thanks for reviewing!

coleenp

Looks good. Thanks for removing this inline include.

coleenp · 2024-10-03T13:54:09Z

src/hotspot/os_cpu/bsd_aarch64/vmStructs_bsd_aarch64.hpp

-  /* Threads (NOTE: incomplete) */                                                                                                   \
-  /******************************/                                                                                                   \
-  nonstatic_field(OSThread,                      _thread_id,                                      OSThread::thread_id_t)             \
-  nonstatic_field(OSThread,                      _unique_thread_id,                               uint64_t)


Ok. I see now why you don't also have to change SA.

stefank · 2024-10-03T14:02:57Z

Thanks for reviewing!

dholmes-ora · 2024-10-04T05:01:18Z

One interesting thing that we found while looking at this is that the AIX port has the _startThread_lock, but it doesn't use it to coordinate the starting of the created thread. It's unclear to me if that's a bug in that port.

Looks like a copy'n'paste bug. AIX starts the new thread in the suspended state the same as Windows. So less sharing of the startThread_lock than I thought at present.

dholmes-ora · 2024-10-04T05:39:12Z

I found that the field is not available on Windows, and it's also left uninitialized when we call create_attached_thread.

I did some archaeology on this one. The thread_type was introduced way back in May 2000 on Linux as it was needed for the newly started thread to determine whether it had to install (and later remove) an alternate signal stack. When we later stopped using alt-signal-stack (I'm going to guess this was when we switched from LinuxThreads to NPTL) the field was no longer used, but meanwhile we used the ThreadType enum to control initial stack sizes. So Windows never had a thread_type field and BSD/AIX likely just blindly copied it from the Linux code.

Attaching a thread never involved using alt-signal-stack so for attached threads it was left uninitialized.

So I think we can clean this up - I will file a RFE.

stefank · 2024-10-04T11:39:49Z

/integrate

openjdk · 2024-10-04T11:43:12Z

Going to push as commit 72ac72f.
Since your change was applied there have been 64 commits pushed to the master branch:

7fa2f22: 8341127: Extra call to MethodHandle::asType from memory segment var handles fails to inline
1bdd79e: 8341261: Tests assume UnlockExperimentalVMOptions is disabled by default
ec020f3: 8340426: ZGC: Move defragment out of the allocation path
a63ac5a: 8340792: -XX:+PrintInterpreter: instructions should only be printed if printing all InterpreterCodelets
3f420fa: 8341451: Remove C2HandleAnonOMOwnerStub
d3139b4: 8341000: Open source some of the AWT Window tests
4ded283: 8338136: Hotspot should support multiple large page sizes on Windows
10402b4: 8341489: ProblemList runtime/cds/appcds/DumpRuntimeClassesTest.java in Xcomp mode
6bc3971: 8341316: [macos] javax/swing/ProgressMonitor/ProgressMonitorEscapeKeyPress.java fails sometimes in macos
e89fd1d: 8341128: open source some 2d graphics tests
... and 54 more: https://git.openjdk.org/jdk/compare/1b46fea59cf8f53b23e5c16a604b4decc8c7dbbe...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-04T11:43:20Z

@stefank Pushed as commit 72ac72f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

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

27a9567

… class

openjdk bot added the rfr Pull request is ready for review label Oct 2, 2024

openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Oct 2, 2024

dholmes-ora approved these changes Oct 3, 2024

View reviewed changes

src/hotspot/share/runtime/osThread.hpp Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label Oct 3, 2024

Add comment to the include of the platform specific class

a3a9073

stefank added 2 commits October 3, 2024 10:37

Move VMStructs fields out of the CPU files

f4eecf2

Move NONCOPYABLE

04260b9

openjdk bot removed the ready Pull request is ready to be integrated label Oct 3, 2024

coleenp reviewed Oct 3, 2024

View reviewed changes

coleenp approved these changes Oct 3, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 3, 2024

openjdk bot added the integrated Pull request has been integrated label Oct 4, 2024

openjdk bot closed this Oct 4, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Oct 4, 2024

openjdk bot removed the rfr Pull request is ready for review label Oct 4, 2024

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

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

Uh oh!

Conversation

stefank commented Oct 2, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Oct 2, 2024

Uh oh!

openjdk bot commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 2, 2024

Uh oh!

mlbridge bot commented Oct 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dholmes-ora commented Oct 3, 2024

Uh oh!

xmas92 commented Oct 3, 2024

Uh oh!

dholmes-ora commented Oct 3, 2024

Uh oh!

xmas92 commented Oct 3, 2024

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

stefank Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

coleenp Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

stefank commented Oct 3, 2024

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

stefank commented Oct 3, 2024

Uh oh!

dholmes-ora commented Oct 4, 2024

Uh oh!

dholmes-ora commented Oct 4, 2024

Uh oh!

stefank commented Oct 4, 2024

Uh oh!

openjdk bot commented Oct 4, 2024

Uh oh!

openjdk bot commented Oct 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

stefank commented Oct 2, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 2, 2024 •

edited

Loading

mlbridge bot commented Oct 2, 2024 •

edited

Loading