[9] RFR(L) 8013267 : move MemberNameTable from native code to Java heap, use to intern MemberNames

[Peter Levart]

On 10/31/2014 07:11 PM, David Chase wrote:
> I found a lurking bug and updated the webrevs — I was mistaken
> about this version having passed the ute tests (but now, for real, it does).
>
> I also added converted Christian’s test code into a jtreg test (which passes):
>
> http://cr.openjdk.java.net/~drchase/8013267/hotspot.05/
> http://cr.openjdk.java.net/~drchase/8013267/jdk.05/

Hi David,

I'll just comment on the JDK side of things.

In Class.ClassData.intern(), ther is a part that synchronizes on the 
elementData (volatile field holding array of Comparable(s)):

2500             synchronized (elementData) {
2501                 final int index = Arrays.binarySearch(elementData, 0, size, memberName);
2502                 if (index >= 0) {
2503                     return (E) elementData[index];
2504                 }
2505                 // Not found, add carefully.
2506                 return add(klass, ~index, memberName, redefined_count);
2507             }


Inside this synchronized block, add() method is called, which can call 
grow() method:

2522             if (oldCapacity + 1 > element_data.length ) {
2523                 // Replacing array with a copy is safe; elements are identical.
2524                 grow(oldCapacity + 1);
2525                 element_data = elementData;
2526             }


grow() method creates a copy of elementData array and replaces it on 
this volatile field (line 2584):

2577         private void grow(int minCapacity) {
2578             // overflow-conscious code
2579             int oldCapacity = elementData.length;
2580             int newCapacity = oldCapacity + (oldCapacity >> 1);
2581             if (newCapacity - minCapacity < 0)
2582                 newCapacity = minCapacity;
2583             // minCapacity is usually close to size, so this is a win:
2584             elementData = Arrays.copyOf(elementData, newCapacity);
2585         }


A concurrent call to intern() can therefore synchronize on a different 
monitor, so two threads will be inserting the element into the same 
array at the same time, Auch!



Also, lazy construction of ClassData instance:

2593     private ClassData<T> classData() {
2594         if (this.classData != null) {
2595             return this.classData;
2596         }
2597         synchronized (this) {
2598             if (this.classData == null) {
2599                 this.classData = new ClassData<>();
2600             }
2601         }
2602         return this.classData;
2603     }


Synchronizes on the j.l.Class instance, which can interfere with user 
synchronization (think synchronized static methods). This dangerous.

Theres an inner class Class.Atomic which is a home for Unsafe machinery 
in j.l.Class. You can add a casClassData method to it and use it to 
atomically install the ClassData instance without synchronized blocks.



Regards, Peter

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20141031/3c2ec093/attachment-0001.html>