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

8265933: Move Java monitor related fields from class Thread to JavaThread #3722

Closed
wants to merge 2 commits into from

Conversation

@pchilano
Copy link
Contributor

@pchilano pchilano commented Apr 27, 2021

Hi,

Please review the following change. Fields in class Thread related to Java object monitors like _current_pending_monitor, _current_pending_monitor_is_from_java, _current_waiting_monitor and _Stalled should be moved to the JavaThread class. Members _OnTrap and _TypeTag were grouped together with _Stalled but are not used so I removed them. It seems they were part of the ObjectMonitor implementation at some point but I couldn't find how they were used.
Tested in mach5 tier1-2.

Thanks,
Patricio


Progress

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

Issue

  • JDK-8265933: Move Java monitor related fields from class Thread to JavaThread

Reviewers

Reviewing

Using git

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

Update a local copy of the PR:
$ git checkout pull/3722
$ git pull https://git.openjdk.java.net/jdk pull/3722/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3722

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3722.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 27, 2021

👋 Welcome back pchilanomate! 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 Apr 27, 2021

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

  • hotspot-runtime

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.

@pchilano pchilano marked this pull request as ready for review Apr 28, 2021
@openjdk openjdk bot added the rfr label Apr 28, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 28, 2021

Webrevs

robehn
robehn approved these changes Apr 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 28, 2021

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

8265933: Move Java monitor related fields from class Thread to JavaThread

Reviewed-by: rehn, dcubed, 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 44 new commits pushed to the master branch:

  • 23180f8: 8266017: Refactor the *klass::array_klass_impl code to separate the non-exception-throwing API
  • f75dd80: 8266230: mark hotspot compiler/c2 tests which ignore VM flags
  • 9df6cc7: 8264678: Incomplete comment in build.tools.generatecharacter.GenerateCharacter
  • 73cfc26: 8266232: compiler.c1.TestRangeCheckEliminated should be run in driver mode
  • 3e1b90a: 8266157: Problem list several awt jtreg tests that fail on macOS 11
  • 3f9879f: 8266190: mark hotspot compiler/codecache tests which ignore VM flags
  • d12e01a: 8264472: Add a test group for running CDS tests with -XX:+VerifySharedSpaces
  • b3b2bb2: 8265773: incorrect jdeps message "jdk8internals" to describe a removed JDK internal API
  • 2780577: 8196415: Disable SHA-1 Signed JARs
  • 21f65f8: 8266206: Build failure after JDK-8264752 with older GCCs
  • ... and 34 more: https://git.openjdk.java.net/jdk/compare/5634f206e5698fc4ae30023714a78273d7df3ee0...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 openjdk bot added the ready label Apr 28, 2021
Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Thumbs up.

@@ -693,8 +663,6 @@ class Thread: public ThreadShadow {
JFR_ONLY(DEFINE_THREAD_LOCAL_OFFSET_JFR;)

public:
volatile intptr_t _Stalled;
volatile int _TypeTag;
ParkEvent * volatile _ParkEvent; // for Object monitors, JVMTI raw monitors,
Copy link
Contributor

@coleenp coleenp Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So are jvmti raw monitors not required to be taken on a Java Thread?

Copy link
Contributor Author

@pchilano pchilano Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Coleen,

So are jvmti raw monitors not required to be taken on a Java Thread?

I thought they were, but JvmtiRawMonitor::raw_enter() has a conditional that checks whether the thread is a JavaThread or not. I looked at the generated jvmti_RawMonitorEnter() method and don't see a straightforward return for non-JavaThreads. They are rather filtered inside a conditional:

  if (Threads::number_of_threads() == 0) {
    transition = false;
  } else {
    this_thread = Thread::current_or_null();
    transition = ((this_thread != NULL) && !this_thread->is_Named_thread());
  }

  if (transition) {
    if (!this_thread->is_Java_thread()) {
      return JVMTI_ERROR_UNATTACHED_THREAD;
    }
    ... // error checking
    err = jvmti_env->RawMonitorEnter(rmonitor);
  } else {
   ... // error checking
   err = jvmti_env->RawMonitorEnter(rmonitor);
  }

If this_thread is NULL or a NamedThread I would have expected a plain return, like other jvmti methods do (e.g. jvmti_GetBytecodes):

  Thread* this_thread = Thread::current_or_null(); 
  if (this_thread == NULL || !this_thread->is_Java_thread()) {
    return JVMTI_ERROR_UNATTACHED_THREAD;
  }

Since the this_thread == NULL case should crash before getting to raw_enter() (unless we are also in the case where Threads::number_of_threads() == 0 in which case we just append the monitor to a list of pending monitors and return), that can't be the reason why raw_enter() receives Thread* instead of JavaThread*. So that leaves the NamedThread case then. I would guess that case is for the VMThread (we might call here when posting jvmti events?).

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Patricio,

Changes look good!

The two unused fields appear to have never been used. They were added, but not used, in JDK 6.0b59 (JDK-5030359). I can only assume they were part of experimental code that never made it into the product.

Thanks,
David

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Apr 29, 2021

Thanks @robehn, @dcubed-ojdk, @dholmes-ora and @coleenp for the reviews!

@pchilano
Copy link
Contributor Author

@pchilano pchilano commented Apr 29, 2021

/integrate

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

@openjdk openjdk bot commented Apr 29, 2021

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

  • 1afbab6: 8263998: Remove mentions of mc region in comments
  • 51b2fb5: 8266299: ProblemList runtime/stringtable/StringTableCleaningTest.java on linux-aarch64 with ZGC
  • 49d0458: 8266288: assert root method not found in witnessed_reabstraction_in_supers is too strong
  • 01415f3: 8266250: WebSocketTest and WebSocketProxyTest call assertEquals(List<byte[]>, List<byte[]>)
  • 5f15666: 8266078: Reader.read(CharBuffer) advances Reader position for read-only Charbuffers
  • 2a03739: 8266014: Regression brought by optimization done with JDK-4926314
  • 6bb71d9: 8264762: ByteBuffer.byteOrder(BIG_ENDIAN).asXBuffer.put(Xarray) and ByteBuffer.byteOrder(nativeOrder()).asXBuffer.put(Xarray) are slow
  • f0f6b0d: 8266027: The diamond finder does not find diamond candidates in field initializers
  • 8072ea5: 8238173: jshell - switch statement with a single default not return cause syntax error
  • c76ce28: 8265842: G1: Introduce API to run multiple separate tasks in a single gangtask
  • ... and 65 more: https://git.openjdk.java.net/jdk/compare/5634f206e5698fc4ae30023714a78273d7df3ee0...master

Your commit was automatically rebased without conflicts.

Pushed as commit 42af7da.

💡 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