JDK-6875393 : JNI itable index cache is broken
  • Type: Bug
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: hs16,6
  • Priority: P2
  • Status: Closed
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2009-08-25
  • Updated: 2011-03-08
  • Resolved: 2011-03-08
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 6 JDK 7 Other
6u18Fixed 7Fixed hs16Fixed
Related Reports
Relates :  
Relates :  
Description
The JNI itable index cache is managed by the following functions
in src/share/vm/oops/instanceKlass.cpp:

instanceKlass::set_cached_itable_index(size_t idnum, int index)
instanceKlass::cached_itable_index(size_t idnum)

These functions were added by the fix for the following bug:

    6404550 1/1 Cannot implement late attach in NetBeans Profiler
                due to missing functionality in JVMTI

The fix for 6404550 was integrated in Mustang-B83.

The cache is used in

src/share/vm/prims/jni.cpp: jni_invoke_nonstatic()

    } else {
      // interface call
      KlassHandle h_holder(THREAD, holder);

      int itbl_index = m->cached_itable_index();
      if (itbl_index == -1) {
        itbl_index = klassItable::compute_itable_index(m);
        m->set_cached_itable_index(itbl_index);
        // the above may have grabbed a lock, 'm' and anything non-handlized can't be used again
      }
      klassOop k = h_recv->klass();
      selected_method = instanceKlass::cast(k)->method_at_itable(h_holder(), itbl_index, CHECK);
    }

Figuring out a method's itable index for a JNI invoke is expensive
so the idea is to cache the itable index on the first JNI invoke
of the method and then reuse it for subsequent invokes.

However, while I was investigating the following bug:

    6419370 4/4 new jmethodID code has tiny holes in synchronization

I noticed that set_cached_itable_index() was allocating a new,
full size cache array for every cache access attempt. On further
investigation, I figured out that the newly allocated cache was
missing the critical assignment to element zero:

    // array length stored in first element, other elements offset by one
    if (indices == NULL || (length = (size_t)indices[0]) <= idnum) {
      size_t size = MAX2(idnum+1, (size_t)idnum_allocated_count());
      int* new_indices = NEW_C_HEAP_ARRAY(int, size+1);
/*MISSING*/ new_indices[0] =(int)size;  // array size held in the first element
      // Copy the existing entries, if any
      size_t i;
      for (i = 0; i < length; i++) {
        new_indices[i+1] = indices[i+1];
      }
      // Set all the rest to -1
      for (i = length; i < size; i++) {
        new_indices[i+1] = -1;
      }
      if (indices != NULL) {
        FreeHeap(indices);  // delete any old indices
      }
      release_set_methods_cached_itable_indices(indices = new_indices);

The interesting part of this bug is that the missing
setting of the cache size means that we allocate a new
cache and free the old cache on every cache access. A
really stressful situation that has helped me reproduce
one of the races described in 6419370.

Comments
EVALUATION http://hg.openjdk.java.net/jdk7/hotspot-comp/hotspot/rev/9601152ccfc1
25-09-2009

EVALUATION http://hg.openjdk.java.net/hsx/hsx16/master/rev/9601152ccfc1
04-09-2009

EVALUATION http://hg.openjdk.java.net/hsx/hsx16/baseline/rev/9601152ccfc1
29-08-2009

EVALUATION http://hg.openjdk.java.net/jdk7/hotspot-rt/hotspot/rev/9601152ccfc1
28-08-2009

SUGGESTED FIX I found the common changeset for HSX-14 and HSX-16 and I've applied the fix relative to that changeset. That will allow me to push this fix to HSX-17 (JDK7), HSX-16 (JDK6-U18) and HSX-14 (OpenJDK6). I think that covers the bases. Here is the context diff: % hg diff src/share/vm/oops/instanceKlass.cpp diff -r 2b4230d1e589 src/share/vm/oops/instanceKlass.cpp --- a/src/share/vm/oops/instanceKlass.cpp Tue Jul 28 13:35:00 2009 -0600 +++ b/src/share/vm/oops/instanceKlass.cpp Tue Aug 25 15:20:26 2009 -0600 @@ -1073,6 +1073,7 @@ if (indices == NULL || (length = (size_t)indices[0]) <= idnum) { size_t size = MAX2(idnum+1, (size_t)idnum_allocated_count()); int* new_indices = NEW_C_HEAP_ARRAY(int, size+1); + new_indices[0] =(int)size; // array size held in the first element // Copy the existing entries, if any size_t i; for (i = 0; i < length; i++) {
25-08-2009

EVALUATION The description pretty much says it all.
25-08-2009

SUGGESTED FIX int* new_indices = NEW_C_HEAP_ARRAY(int, size+1); // Add the following line: new_indices[0] =(int)size; // array size held in the first element
25-08-2009