Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8264016: [JVMCI] add some thread local fields for use by JVMCI #3147

Closed

Conversation

@tkrodriguez
Copy link
Contributor

@tkrodriguez tkrodriguez commented Mar 23, 2021


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8264016: [JVMCI] add some thread local fields for use by JVMCI

Reviewers

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3147/head:pull/3147
$ git checkout pull/3147

To update a local copy of the PR:
$ git checkout pull/3147
$ git pull https://git.openjdk.java.net/jdk pull/3147/head

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 23, 2021

👋 Welcome back never! 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
Copy link

@openjdk openjdk bot commented Mar 23, 2021

@tkrodriguez The following label will be automatically applied to this pull request:

  • hotspot

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.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 23, 2021

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Tom,

Is it feasible to create a JVMCI helper side-object that is only created when needed, rather than embedding all the fields directly in the JavaThread instance?

Thanks,
David

// Fast thread locals for use by JVMCI
intptr_t* _jvmci_reserved0;
intptr_t* _jvmci_reserved1;
oop _jvmci_reserved_oop0;
Copy link
Member

@dholmes-ora dholmes-ora Mar 23, 2021

Can this use OopStorage? We've been getting rid of oop fields and the corresponding oops_do support.

Copy link
Contributor Author

@tkrodriguez tkrodriguez Mar 24, 2021

Wouldn't using OopStorage require an extra level of indirection for the field?

Copy link
Contributor

@coleenp coleenp Mar 24, 2021

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.

Copy link
Contributor Author

@tkrodriguez tkrodriguez Mar 24, 2021

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.

Copy link
Contributor

@coleenp coleenp Mar 24, 2021

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.

Copy link
Contributor Author

@tkrodriguez tkrodriguez Mar 25, 2021

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?

Copy link
Contributor

@coleenp coleenp Mar 25, 2021

Ok!

@tkrodriguez
Copy link
Contributor Author

@tkrodriguez tkrodriguez commented Mar 24, 2021

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.
Out of curiosity I'd used the clang option -Xclang -fdump-record-layouts to look at JavaThread in 8, 11 and 17. It's definitely getting fatter. 8 is 1024, 11 is 1264 and 17 is 1408, plus the huge alignment wastage caused by biased locking, though I guess that's not a problem in 17. Anyway, I could probably get back the space I'm using by rearranging some of the fields of JavaThread. There's a lot of wastage from switching between pointer, int and bool. 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.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 24, 2021

Mailing list message from David Holmes on hotspot-dev:

Hi Tom,

On 24/03/2021 4:06 pm, Tom Rodriguez wrote:

On Tue, 23 Mar 2021 06:28:55 GMT, David Holmes <dholmes at openjdk.org> wrote:

8264016: [JVMCI] add some thread local fields for use by JVMCI

Hi Tom,

Is it feasible to create a JVMCI helper side-object that is only created when needed, rather than embedding all the fields directly in the JavaThread instance?

Thanks,
David

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.

Understood.

Out of curiosity I'd used the clang option ``-Xclang -fdump-record-layouts`` to look at JavaThread in 8, 11 and 17. It's definitely getting fatter. 8 is 1024, 11 is 1264 and 17 is 1408, plus the huge alignment wastage caused by biased locking, though I guess that's not a problem in 17. Anyway, I could probably get back the space I'm using by rearranging some of the fields of JavaThread. There's a lot of wastage from switching between pointer, int and bool. 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.

Thanks for that info - that's quite a large growth since 11. And
Biased-locking is still in 17 (removal deferred to 18) so we're still
paying for additional alignment there.

Does the C++ compiler attempt to do any field packing or is everything
laid out as we write it? I could see a couple of int fields in Thread
that might trigger gaps, and then in JavaThread there is a very suspect
layout here:

// suspend/resume support
volatile bool _suspend_equivalent; // Suspend equivalent
condition
jint _in_deopt_handler; // count of deoptimization
// handlers thread is in
volatile bool _doing_unsafe_access; // Thread may fault
due to unsafe access
bool _do_not_unlock_if_synchronized;

I don't know how frugal the C++ compiler actually is with bools, but
perhaps some simple reordering there might reclaim some space?

Obviously I'd prefer not to have Thread/JavaThread keep growing, but
just because you're the most recent requestor doesn't mean the burden
should fall to you to fix the overall layout problem. So I'm okay with
what you propose and will file a RFE to have someone look into
optimising the layout to see if we can recoup some space.

src/hotspot/share/runtime/thread.hpp line 1020:

1018: intptr_t* _jvmci_reserved0;
1019: intptr_t* _jvmci_reserved1;
1020: oop _jvmci_reserved_oop0;

Can this use OopStorage? We've been getting rid of oop fields and the corresponding oops_do support.

Wouldn't using OopStorage require an extra level of indirection for the field?

Possibly - but there has been a very strong move to using oopStorage in
any case. Probably best to ask Erik O./Kim/Coleen about that.

Thanks,
David
-----

@openjdk
Copy link

@openjdk openjdk bot commented Mar 24, 2021

@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:

8264016: [JVMCI] add some thread local fields for use by JVMCI

Reviewed-by: dholmes, iklam, coleenp

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 master branch:

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 openjdk bot added the ready label Mar 24, 2021
@pron
Copy link
Member

@pron pron commented Mar 24, 2021

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.

iklam
iklam approved these changes Mar 24, 2021
Copy link
Member

@iklam iklam left a comment

LGTM

Copy link
Contributor

@coleenp coleenp left a 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.

@tkrodriguez
Copy link
Contributor Author

@tkrodriguez tkrodriguez commented Mar 24, 2021

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 char[64] in the HandshakeState and lots of statistics that could live elsewhere. Is there a bug I could attach my observations to? It's nothing earth shattering but the clang reported layout is interesting to see.

@coleenp
Copy link
Contributor

@coleenp coleenp commented Mar 24, 2021

I have a fix for the Mutex in HandshakeState. Here's a bug for you to fill in. That would be great.
https://bugs.openjdk.java.net/browse/JDK-8264145

@coleenp
Copy link
Contributor

@coleenp coleenp commented Mar 24, 2021

One of the things we've talked about is making the hierarchy be something like:
Thread
NamedThread
-> WatcherThread
SafepointAwareThread
-> CompilerThread
-> JavaThread

So that CompilerThread isn't a JavaThread. But we're not going to do that very soon.

@tkrodriguez
Copy link
Contributor Author

@tkrodriguez tkrodriguez commented Mar 24, 2021

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?

@tkrodriguez
Copy link
Contributor Author

@tkrodriguez tkrodriguez commented Mar 30, 2021

Thanks for the reviews. I'll defer any loom related issues to some future date.

@tkrodriguez
Copy link
Contributor Author

@tkrodriguez tkrodriguez commented Mar 30, 2021

/integrate

@openjdk openjdk bot closed this Mar 30, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 30, 2021

@tkrodriguez Since your change was applied there have been 121 commits pushed to the master branch:

  • 8cf1c62: 8263754: HexFormat 'fromHex' methods should be static
  • a5d7de2: 8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec
  • 128c0c9: 8248418: jpackage fails to extract main class and version from app module linked in external runtime
  • fd45694: 8264344: Outdated links in JavaComponentAccessibility.m
  • f17ea9e: 8262899: TestRedirectLinks fails
  • 963f1fc: 8264309: JFR: Improve .jfc parser
  • 364cce1: 8264332: Use the blessed modifier order in jdk.charsets
  • fbbd98b: 8264029: Replace uses of StringBuffer with StringBuilder in java.base
  • 019080e: 8264268: Don't use oop types for derived pointers
  • 3516c26: 8263591: Two C2 compiler phases with the name "after matching"
  • ... and 111 more: https://git.openjdk.java.net/jdk/compare/6b4c654186e49528eeae7249fdcd0f2d1a98b3ad...master

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants