8265489: Stress test times out because of long ObjectSynchronizer::mo…

…nitors_iterate(...) operation Reviewed-by: dcubed
openjdk · Sep 8, 2021 · a5e4def · a5e4def · openjdk-notifier · Sep 8, 2021
1 parent 9b5991e
commit a5e4def
Show file tree

Hide file tree

Showing 5 changed files with 40 additions and 47 deletions.
diff --git a/src/hotspot/share/prims/jvmtiEnvBase.cpp b/src/hotspot/share/prims/jvmtiEnvBase.cpp
@@ -717,8 +717,8 @@ JvmtiEnvBase::get_owned_monitors(JavaThread *calling_thread, JavaThread* java_th
   }
 
   // Get off stack monitors. (e.g. acquired via jni MonitorEnter).
-  JvmtiMonitorClosure jmc(java_thread, calling_thread, owned_monitors_list, this);
-  ObjectSynchronizer::monitors_iterate(&jmc);
+  JvmtiMonitorClosure jmc(calling_thread, owned_monitors_list, this);
+  ObjectSynchronizer::monitors_iterate(&jmc, java_thread);
   err = jmc.error();
 
   return err;
@@ -1450,34 +1450,32 @@ JvmtiMonitorClosure::do_monitor(ObjectMonitor* mon) {
     // to the list.
     return;
   }
-  if (mon->owner() == _java_thread ) {
-    // Filter out on stack monitors collected during stack walk.
-    oop obj = mon->object();
-    bool found = false;
-    for (int j = 0; j < _owned_monitors_list->length(); j++) {
-      jobject jobj = ((jvmtiMonitorStackDepthInfo*)_owned_monitors_list->at(j))->monitor;
-      oop check = JNIHandles::resolve(jobj);
-      if (check == obj) {
-        // On stack monitor already collected during the stack walk.
-        found = true;
-        break;
-      }
+  // Filter out on stack monitors collected during stack walk.
+  oop obj = mon->object();
+  bool found = false;
+  for (int j = 0; j < _owned_monitors_list->length(); j++) {
+    jobject jobj = ((jvmtiMonitorStackDepthInfo*)_owned_monitors_list->at(j))->monitor;
+    oop check = JNIHandles::resolve(jobj);
+    if (check == obj) {
+      // On stack monitor already collected during the stack walk.
+      found = true;
+      break;
     }
-    if (found == false) {
-      // This is off stack monitor (e.g. acquired via jni MonitorEnter).
-      jvmtiError err;
-      jvmtiMonitorStackDepthInfo *jmsdi;
-      err = _env->allocate(sizeof(jvmtiMonitorStackDepthInfo), (unsigned char **)&jmsdi);
-      if (err != JVMTI_ERROR_NONE) {
-        _error = err;
-        return;
-      }
-      Handle hobj(Thread::current(), obj);
-      jmsdi->monitor = _env->jni_reference(_calling_thread, hobj);
-      // stack depth is unknown for this monitor.
-      jmsdi->stack_depth = -1;
-      _owned_monitors_list->append(jmsdi);
+  }
+  if (found == false) {
+    // This is off stack monitor (e.g. acquired via jni MonitorEnter).
+    jvmtiError err;
+    jvmtiMonitorStackDepthInfo *jmsdi;
+    err = _env->allocate(sizeof(jvmtiMonitorStackDepthInfo), (unsigned char **)&jmsdi);
+    if (err != JVMTI_ERROR_NONE) {
+      _error = err;
+      return;
     }
+    Handle hobj(Thread::current(), obj);
+    jmsdi->monitor = _env->jni_reference(_calling_thread, hobj);
+    // stack depth is unknown for this monitor.
+    jmsdi->stack_depth = -1;
+    _owned_monitors_list->append(jmsdi);
   }
 }
 

diff --git a/src/hotspot/share/prims/jvmtiEnvBase.hpp b/src/hotspot/share/prims/jvmtiEnvBase.hpp
@@ -639,17 +639,15 @@ class ResourceTracker : public StackObj {
 // Jvmti monitor closure to collect off stack monitors.
 class JvmtiMonitorClosure: public MonitorClosure {
  private:
-  JavaThread *_java_thread;
   JavaThread *_calling_thread;
   GrowableArray<jvmtiMonitorStackDepthInfo*> *_owned_monitors_list;
   jvmtiError _error;
   JvmtiEnvBase *_env;
 
  public:
-  JvmtiMonitorClosure(JavaThread* thread, JavaThread *calling_thread,
+  JvmtiMonitorClosure(JavaThread *calling_thread,
                       GrowableArray<jvmtiMonitorStackDepthInfo*> *owned_monitors,
                       JvmtiEnvBase *env) {
-    _java_thread = thread;
     _calling_thread = calling_thread;
     _owned_monitors_list = owned_monitors;
     _error = JVMTI_ERROR_NONE;

diff --git a/src/hotspot/share/runtime/synchronizer.cpp b/src/hotspot/share/runtime/synchronizer.cpp
@@ -972,10 +972,13 @@ JavaThread* ObjectSynchronizer::get_lock_owner(ThreadsList * t_list, Handle h_ob
 
 // Visitors ...
 
-void ObjectSynchronizer::monitors_iterate(MonitorClosure* closure) {
+void ObjectSynchronizer::monitors_iterate(MonitorClosure* closure, JavaThread* thread) {
   MonitorList::Iterator iter = _in_use_list.iterator();
   while (iter.has_next()) {
     ObjectMonitor* mid = iter.next();
+    if (mid->owner() != thread) {
+      continue;
+    }
     if (!mid->is_being_async_deflated() && mid->object_peek() != NULL) {
       // Only process with closure if the object is set.
 
@@ -1461,9 +1464,7 @@ class ReleaseJavaMonitorsClosure: public MonitorClosure {
  public:
   ReleaseJavaMonitorsClosure(JavaThread* thread) : _thread(thread) {}
   void do_monitor(ObjectMonitor* mid) {
-    if (mid->owner() == _thread) {
-      (void)mid->complete_exit(_thread);
-    }
+    (void)mid->complete_exit(_thread);
   }
 };
 
@@ -1486,7 +1487,7 @@ void ObjectSynchronizer::release_monitors_owned_by_thread(JavaThread* current) {
   assert(current == JavaThread::current(), "must be current Java thread");
   NoSafepointVerifier nsv;
   ReleaseJavaMonitorsClosure rjmc(current);
-  ObjectSynchronizer::monitors_iterate(&rjmc);
+  ObjectSynchronizer::monitors_iterate(&rjmc, current);
   assert(!current->has_pending_exception(), "Should not be possible");
   current->clear_pending_exception();
 }

diff --git a/src/hotspot/share/runtime/synchronizer.hpp b/src/hotspot/share/runtime/synchronizer.hpp
@@ -133,7 +133,7 @@ class ObjectSynchronizer : AllStatic {
 
   // JNI detach support
   static void release_monitors_owned_by_thread(JavaThread* current);
-  static void monitors_iterate(MonitorClosure* m);
+  static void monitors_iterate(MonitorClosure* m, JavaThread* thread);
 
   // Initialize the gInflationLocks
   static void initialize();

diff --git a/src/hotspot/share/services/threadService.cpp b/src/hotspot/share/services/threadService.cpp
@@ -618,18 +618,14 @@ void StackFrameInfo::print_on(outputStream* st) const {
 class InflatedMonitorsClosure: public MonitorClosure {
 private:
   ThreadStackTrace* _stack_trace;
-  Thread* _thread;
 public:
-  InflatedMonitorsClosure(Thread* t, ThreadStackTrace* st) {
-    _thread = t;
+  InflatedMonitorsClosure(ThreadStackTrace* st) {
     _stack_trace = st;
   }
   void do_monitor(ObjectMonitor* mid) {
-    if (mid->owner() == _thread) {
-      oop object = mid->object();
-      if (!_stack_trace->is_owned_monitor_on_stack(object)) {
-        _stack_trace->add_jni_locked_monitor(object);
-      }
+    oop object = mid->object();
+    if (!_stack_trace->is_owned_monitor_on_stack(object)) {
+      _stack_trace->add_jni_locked_monitor(object);
     }
   }
 };
@@ -688,8 +684,8 @@ void ThreadStackTrace::dump_stack_at_safepoint(int maxDepth) {
   if (_with_locked_monitors) {
     // Iterate inflated monitors and find monitors locked by this thread
     // not found in the stack
-    InflatedMonitorsClosure imc(_thread, this);
-    ObjectSynchronizer::monitors_iterate(&imc);
+    InflatedMonitorsClosure imc(this);
+    ObjectSynchronizer::monitors_iterate(&imc, _thread);
   }
 }