8269661: JNI_GetStringCritical does not lock char array

8269650: Optimize gc-locker in [Get|Release]StringCritical for latin string

Reviewed-by: dholmes, tschatzl

src/hotspot/share/gc/shared/stringdedup/stringDedup.cpp

@@ -44,6 +44,7 @@
 #include "oops/oopsHierarchy.hpp"
 #include "runtime/globals.hpp"
 #include "runtime/mutexLocker.hpp"
+#include "runtime/orderAccess.hpp"
 #include "runtime/safepoint.hpp"
 #include "runtime/thread.hpp"
 #include "utilities/debug.hpp"
@@ -107,6 +108,19 @@ void StringDedup::threads_do(ThreadClosure* tc) {
   tc->do_thread(_processor);
 }
 
+void StringDedup::forbid_deduplication(oop java_string) {
+  assert(is_enabled(), "precondition");
+  if (java_lang_String::deduplication_forbidden(java_string)) {
+    // DCLP - we don't want a caller's access to the value array to float
+    // before the check; string dedup could change the value and another
+    // thread could set the flag, and this thread uses a stale value.
+    OrderAccess::acquire();
+  } else {
+    MutexLocker ml(StringDedupIntern_lock, Mutex::_no_safepoint_check_flag);
+    java_lang_String::set_deduplication_forbidden(java_string);
+  }
+}
+
 void StringDedup::notify_intern(oop java_string) {
   assert(is_enabled(), "precondition");
   // A String that is interned in the StringTable must not later have its
@@ -114,10 +128,7 @@ void StringDedup::notify_intern(oop java_string) {
   // can still add the byte array to the dedup table for sharing, so add the
   // string to the pending requests.  Triggering request processing is left
   // to the next GC.
-  {
-    MutexLocker ml(StringDedupIntern_lock, Mutex::_no_safepoint_check_flag);
-    java_lang_String::set_deduplication_forbidden(java_string);
-  }
+  forbid_deduplication(java_string);
   StorageUse* requests = Processor::storage_for_requests();
   oop* ref = requests->storage()->allocate();
   if (ref != nullptr) {

src/hotspot/share/gc/shared/stringdedup/stringDedup.hpp

@@ -147,7 +147,17 @@ public:
   // precondition: is_enabled()
   static void threads_do(ThreadClosure* tc);
 
+  // Marks the String as not being subject to deduplication.  This can be
+  // used to prevent deduplication of Strings whose value array must remain
+  // stable and cannot be replaced by a shared duplicate.  Must be called
+  // before obtaining the value array; this function provides an acquire
+  // barrier.
+  // precondition: is_enabled()
+  // precondition: java_string is a Java String object.
+  static void forbid_deduplication(oop java_string);
+
   // Notify that a String is being added to the StringTable.
+  // Implicity forbids deduplication of the String.
   // precondition: is_enabled()
   // precondition: java_string is a Java String object.
   static void notify_intern(oop java_string);

src/hotspot/share/prims/jni.cpp

@@ -44,6 +44,7 @@
 #include "compiler/compiler_globals.hpp"
 #include "gc/shared/collectedHeap.hpp"
 #include "gc/shared/gcLocker.inline.hpp"
+#include "gc/shared/stringdedup/stringDedup.hpp"
 #include "interpreter/linkResolver.hpp"
 #include "jfr/jfrEvents.hpp"
 #include "jfr/support/jfrThreadId.hpp"
@@ -2842,19 +2843,45 @@ HOTSPOT_JNI_RELEASEPRIMITIVEARRAYCRITICAL_RETURN();
 JNI_END
 
 
+static typeArrayOop lock_gc_or_pin_string_value(JavaThread* thread, oop str) {
+  if (Universe::heap()->supports_object_pinning()) {
+    // Forbid deduplication before obtaining the value array, to prevent
+    // deduplication from replacing the value array while setting up or in
+    // the critical section.  That would lead to the release operation
+    // unpinning the wrong object.
+    if (StringDedup::is_enabled()) {
+      NoSafepointVerifier nsv;
+      StringDedup::forbid_deduplication(str);
+    }
+    typeArrayOop s_value = java_lang_String::value(str);
+    return (typeArrayOop) Universe::heap()->pin_object(thread, s_value);
+  } else {
+    Handle h(thread, str);      // Handlize across potential safepoint.
+    GCLocker::lock_critical(thread);
+    return java_lang_String::value(h());
+  }
+}
+
+static void unlock_gc_or_unpin_string_value(JavaThread* thread, oop str) {
+  if (Universe::heap()->supports_object_pinning()) {
+    typeArrayOop s_value = java_lang_String::value(str);
+    Universe::heap()->unpin_object(thread, s_value);
+  } else {
+    GCLocker::unlock_critical(thread);
+  }
+}
+
 JNI_ENTRY(const jchar*, jni_GetStringCritical(JNIEnv *env, jstring string, jboolean *isCopy))
   HOTSPOT_JNI_GETSTRINGCRITICAL_ENTRY(env, string, (uintptr_t *) isCopy);
-  oop s = lock_gc_or_pin_object(thread, string);
-  typeArrayOop s_value = java_lang_String::value(s);
-  bool is_latin1 = java_lang_String::is_latin1(s);
-  if (isCopy != NULL) {
-    *isCopy = is_latin1 ? JNI_TRUE : JNI_FALSE;
-  }
+  oop s = JNIHandles::resolve_non_null(string);
   jchar* ret;
-  if (!is_latin1) {
+  if (!java_lang_String::is_latin1(s)) {
+    typeArrayOop s_value = lock_gc_or_pin_string_value(thread, s);
     ret = (jchar*) s_value->base(T_CHAR);
+    if (isCopy != NULL) *isCopy = JNI_FALSE;
   } else {
     // Inflate latin1 encoded string to UTF16
+    typeArrayOop s_value = java_lang_String::value(s);
     int s_len = java_lang_String::length(s, s_value);
     ret = NEW_C_HEAP_ARRAY_RETURN_NULL(jchar, s_len + 1, mtInternal);  // add one for zero termination
     /* JNI Specification states return NULL on OOM */
@@ -2864,6 +2891,7 @@ JNI_ENTRY(const jchar*, jni_GetStringCritical(JNIEnv *env, jstring string, jbool
       }
       ret[s_len] = 0;
     }
+    if (isCopy != NULL) *isCopy = JNI_TRUE;
   }
  HOTSPOT_JNI_GETSTRINGCRITICAL_RETURN((uint16_t *) ret);
   return ret;
@@ -2872,15 +2900,16 @@ JNI_END
 
 JNI_ENTRY(void, jni_ReleaseStringCritical(JNIEnv *env, jstring str, const jchar *chars))
   HOTSPOT_JNI_RELEASESTRINGCRITICAL_ENTRY(env, str, (uint16_t *) chars);
-  // The str and chars arguments are ignored for UTF16 strings
   oop s = JNIHandles::resolve_non_null(str);
   bool is_latin1 = java_lang_String::is_latin1(s);
   if (is_latin1) {
     // For latin1 string, free jchar array allocated by earlier call to GetStringCritical.
     // This assumes that ReleaseStringCritical bookends GetStringCritical.
     FREE_C_HEAP_ARRAY(jchar, chars);
+  } else {
+    // For non-latin1 string, drop the associated gc-locker/pin.
+    unlock_gc_or_unpin_string_value(thread, s);
   }
-  unlock_gc_or_unpin_object(thread, str);
 HOTSPOT_JNI_RELEASESTRINGCRITICAL_RETURN();
 JNI_END