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

[David Chase]

Thanks very much, I shall attend to these irregularities.

David

On 2014-10-31, at 6:01 PM, Peter Levart <peter.levart at gmail.com> wrote:

> 
> 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
>