Code Review for 6704010

Prepared by:Volker H. Simonis on Thu Dec 2 12:17:18 CET 2010
Workspace:/share/software/Java/OpenJDK/hotspot
Compare against: http://hg.openjdk.java.net/jdk7/jdk7/hotspot-comp
Summary of changes: 13 lines changed: 11 ins; 0 del; 2 mod; 1241 unchg
Patch of changes: 6704010.patch
Author comments:
This problem occurs very rarely and it is therefore hardly possible to reliably reproduce it. However after looking at it for a while I think I found the cause of the problem:

The SignatureHandlerLibrary class uses two static GrowableArrays, '_handlers' and '_fingerprints', to store method fingerprints along with their corresponding signature handlers. But GrowableArrays are are NOT synchronized in any way if accessed from multiple threads. To avoid races it is therefore necessary to synchronize different threads which access the same GrowableArray. This is done for most of the code in 'SignatureHandlerLibrary::add()' which is protected by 'MutexLocker mu(SignatureHandlerLibrary_lock)'.

However the assertion at the end of the method is outside the scope of this MutexLocker which can lead to an illegal view on the corresponding GrowableArray data structures. Here'S what may happen in detail:

  • thread one enters the section protected by the MutexLocker and adds another fingerprint/handler to the growable arrays by calling 'GrowableArray::append()'. This can lead to an implicit call to 'GrowableArray::grow()' if the arrays size exhausted. In the 'grow()' method a new data array will be allocated, the contents of the old array will be copied into the new one and then (1) the storage occupied by the old array will be freed and (2) the address of the new storage will be assigned to the internal data pointer. Between (1) and (2) there's a small (like once every six month or so:) chance that the thread will be unscheduled and another thread is allocating the memory which has just been freed.
  • thread two is just in the assertion code and trying to query the GrowableArrays '_handlers' and '_fingerprints' by calling 'GrowableArray::find()'. If this happens right in the interval between the points (1) and (2) described above, the 'find()' method may return incorrect results which can spuriously trigger the assertion.

Fixing this can be easily done by enclosing the assertion into a region which is protected by the same Mutex like the main body of 'SignatureHandlerLibrary::add()'.

As I wrote, I couldn't reproduce this in the HostSpot, so no regression test. I was however able to reproduce the described scenario with a standalone GrowableArray class and a small test program consisting of one writer thread which appends a constant value to a GrowableArray and ten reader threads which are constantly trying to find that special value in the array on a dual core x86_64 Linux box.

As a last side note, the problem could also be made even more unlikely, by changing the implementation of 'GrowableArray::grow()' from:

template void GrowableArray::grow(int j) {
    int old_max = _max;
    ...
    if (on_C_heap() && _data != NULL) {
      FreeHeap(_data);
    }
    _data = newData;
}
to:
template void GrowableArray::grow(int j) {
    int old_max = _max;
    ...
    _old_data = _data;
    _data = newData;
    if (on_C_heap() && _old_data != NULL) {
      FreeHeap(_old_data);
    }
}

On the other hand that's still not 100% safe ('_data' is not volatile, other memory models, ..) and I think GrowableArray is definitely not intended for parallel access and therefore such a change may unnecessarily hide other problems where this precondition is violated. So maybe nulling out the '_data' array before freeing it may be a good idea in the debug build to catch such errors!

Bug id: 6704010 Internal Error (src/share/vm/interpreter/interpreterRuntime.cpp:1106)
Legend: Modified file
Deleted file
New file

Cdiffs Udiffs Sdiffs Frames Old New Patch Raw src/share/vm/interpreter/interpreterRuntime.cpp

13 lines changed: 11 ins; 0 del; 2 mod; 1241 unchg

This code review page was prepared using /usr/local/bin/webrev (vers 23.18-hg).