8264016: [JVMCI] add some thread local fields for use by JVMCI#3147
Conversation
👋 Welcome back never! A progress list of the required criteria for merging this PR into |
@tkrodriguez The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
openjdkbot added hotspot 
| // Fast thread locals for use by JVMCI | ||
| intptr_t* _jvmci_reserved0; | ||
| intptr_t* _jvmci_reserved1; | ||
| oop _jvmci_reserved_oop0; |
There was a problem hiding this comment.
Can this use OopStorage? We've been getting rid of oop fields and the corresponding oops_do support.
There was a problem hiding this comment.
Wouldn't using OopStorage require an extra level of indirection for the field?
There was a problem hiding this comment.
It was https://bugs.openjdk.java.net/browse/JDK-8244997 - you were co-author :)
Maybe this jvmci_reserved_oop0 won't crash for the same reasons. I don't know that.
I still would like to not see 45 lines of declarations for JVMCI added to JavaThread. These should be in a separate header file and declared, as in https://bugs.openjdk.java.net/browse/JDK-8137018. If you promise to fix 8137018, I'm fine with this change.
There was a problem hiding this comment.
co-author is a strong word. :) But that does ring a bell. Why was threadObj problematic but the other existing oop fields were not? JavaThread::oops_do_no_frames visits a lot of roots that aren't OopStorage.
I can tackle JDK-8137018. I think we'll need to add an alias mechanism to vmStructs_jvmci.cpp to maintain backward compatibility but that's fairly straightforward.
There was a problem hiding this comment.
well, I didn't even list myself as author of that one. _threadObj was a problem because some code (like thread dump management code) was accessing it from a terminating thread and the barriers were messed up. It was more complicated than that. I don't know if this will be an issue for these declarations, and if you hide them in another place, we'll never know.
There was a problem hiding this comment.
I see. These fields will only be accessed from generated code so I don't think there are the same runtime considerations with them. Obviously it will be our problem to diagnose and fix if issues like that crop up. We just don't want to break an obvious invariants that would require the use of OopStorage. Do you now approve of these changes?
Well the goal is to have storage that is only one or two loads away so adding a level of indirection isn't great. Many of the other JVMCI fields aren't really performance critical so they could be an extra indirection away but that's not a compatible JVMCI change since they are directly written by Graal for deopt. It also wouldn't save much space. I know there is sensitivity around making JavaThread larger so I tried not to go too crazy. Obviously I'd prefer to stick with what I have. |
Mailing list message from David Holmes on hotspot-dev: Hi Tom, On 24/03/2021 4:06 pm, Tom Rodriguez wrote:
Understood.
Thanks for that info - that's quite a large growth since 11. And Does the C++ compiler attempt to do any field packing or is everything // suspend/resume support I don't know how frugal the C++ compiler actually is with bools, but Obviously I'd prefer not to have Thread/JavaThread keep growing, but
Possibly - but there has been a very strong move to using oopStorage in Thanks, |
@tkrodriguez 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: 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 61 new commits pushed to the
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 |
Note that in Loom, j.l.Thread is no longer tied to a JavaThread, and could migrate among JavaThreads, potentially at any safepoint, unless specifically pinned. Thread-local information that is logically associated with the j.l.Thread can be placed as a field on JavaThread if it's only set and used while the j.l.Thread is mounted on the JavaThread (i.e. between consecutive safepoints); otherwise, it should go on the j.l.Thread class. |
There was a problem hiding this comment.
We made the _threadObj field in JavaThread an OopStorage to avoid a crash during thread deletion. It's not recommended to add oops directly to runtime data structures. I have to dig up the bug first.
I haven't done a deep analysis of how much could be recovered but I could look into reducing the overall size JavaThread by > repacking if it makes adding these fields more palatable.
We have this sort of on our (internal) list with other improvements, so don't do anything here.
We also had a JVMCI bug that suggested adding these fields to a side .hpp file and referring to it in JavaThread by that container. Like HandshakeState. You can optimize the field layout inside this as you want.
I'm marking "Request changes" until I've had time to dig up the bug.
No the C++ compiler just emits them in the declared order. Since we group the field declarations by their relatedness it's not easy to get a completely good overall packing. There's actually less direct waste than I expected in 17, though there are lots of cases where int are used to stored bools. clang doesn't appear to optimize the storage for enums so there are lots of small enums stored in ints instead of bytes. I believe gcc will use the smallest storage available. There's also dubious stuff like the |
I have a fix for the Mutex in HandshakeState. Here's a bug for you to fill in. That would be great. |
One of the things we've talked about is making the hierarchy be something like: So that CompilerThread isn't a JavaThread. But we're not going to do that very soon. |
Regarding loom, it seems like there would have to be a major migration of lots of state into java.lang.Thread. Or some more explicit backup and restores of various chunks of JavaThread during migration. I know from previous discussions that things like the deferred_locals are problematic but JFR and any TLAB statistics/allocation tracking also seems problematic. Is there some systematic solution to the general problem? If these fields needed to live in Thread, then we could inject these fields into java.lang.Thread when JVMCI is enabled. The access wouldn't have to be hugely different. Is a resolution to the loom issue something that can be deferred? |
Thanks for the reviews. I'll defer any loom related issues to some future date. |
/integrate |
openjdkbot added integrated @tkrodriguez Since your change was applied there have been 121 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 182b11c. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Progress
Issue
Reviewers
Download
To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3147/head:pull/3147$ git checkout pull/3147To update a local copy of the PR:
$ git checkout pull/3147$ git pull https://git.openjdk.java.net/jdk pull/3147/head