Re: RFR(S): 8131129: Attempt to define a duplicate BMH$Species class

[Peter Levart]

Hi Michael, Vladimir,

I have finally managed to look at the code on a bigger screen and I 
think your latest webrev is correct now. But it feels like it works 
around the fact that BMH subclass definition (just definition, not 
initialization) is not atomic. I asked myself why normal class loading 
doesn't suffer from this issue. It's because ClassLoader basically does 
class definition in atomic way:

    protected Class<?> loadClass(String name, boolean resolve)
        throws ClassNotFoundException
    {
        synchronized (getClassLoadingLock(name)) {
            // First, check if the class has already been loaded
            Class<?> c = findLoadedClass(name);
            if (c == null) {
                try {
                    if (parent != null) {
                        c = parent.loadClass(name, false);
                    } else {
                        c = findBootstrapClassOrNull(name);
                    }
                } catch (ClassNotFoundException e) {
                    // ClassNotFoundException thrown if class not found
                    // from the non-null parent class loader
                }

                if (c == null) {
                    // If still not found, then invoke findClass in order
                    // to find the class.
                    c = findClass(name);
                }
            }
            if (resolve) {
                resolveClass(c);
            }
            return c;
        }
    }


...so, under the lock that is keyed by class name, it 1st tries to 
'findLoadedClass' and if it is not there, it defines it.

If defining the BMH subclass worked the same way and decoupled class 
initialization from the definition, there would be no duplicate 
definition attempts and a work-around would not be needed. If a BMH 
subclass was defined successfully, but its initialization failed, we 
would not attempt to define it once more next time, we would just try to 
extract the static field value from corrupted class and fail with 
similar exception as the 1st time (or with exception that is chosen by 
VM when trying to use such class, which I think is more correct).

Here's what I am thinking, in code:

    http://cr.openjdk.java.net/~plevart/jdk9-dev/BMH.race/webrev.02/

Now that definition of BMH subclass is atomic, caching of SpeciesData 
can be simplified. We don't need special placeholder instances as locks 
and synchronized static methods. To make BMH subclass definition atomic, 
we can leverage CHM.computeIfAbsent that does the similar "placeholder" 
dance, but in much more sophisticated way. BMH logic is much more 
straightforward and easier to grasp that way.

So what do you think of this version. Your version is logically correct 
too, so you decide which one is better.

Regards, Peter

On 10/29/2015 04:20 PM, Michael Haupt wrote:
Hi Vladimir, Peter,

once more, thanks for all your comments. The revised webrev is at 
http://cr.openjdk.java.net/~mhaupt/8131129/webrev.01/.

Best,

Michael

Am 28.10.2015 um 17:37 schrieb Vladimir Ivanov <vladimir.x.iva...@oracle.com>:

Peter,

Hi Vladimir, I think you are right. The proposed fix alone even
increases the chance of duplicate class definition attempts as it delays
registration in cache to after whole clinit successfully finishes. I
would still keep this initialization rearrangement as it fixes the
possible publication race too. If the source of 1st exception is from
clinit method execution, then it would be best to skip forced class
initialization and delay it to its 1st use. Eager initialization does
not serve any purpose if the registration into cache is performed out of
clinit method as proposed in the fix.

To summarise: the proposed initialization fix would have to be
accompanied with removal of forced class initialization to achieve the
effect of your option (1) below.
There's no benefit from skipping eager initialization - if class initialization 
fails, the class becomes unusable anyway.

The idea was to split class init and SPECIES_DATA field init. It would allow to 
retry Species_* class init later once the initial attempt failed. But there's 
not much value is that right now, since VM doesn't unload generated adapters. 
So, once code cache is exhausted there's not much JVM can do about it.

Also, publishing not fully initialized Species_* class requires all users to 
check whether the initialization is completed, which can be too much from 
performance perspective.

So, I'm in favor of modified (3) Michael proposed.

Best regards,
Vladimir Ivanov

Regards, Peter

Michael, Peter,

Thanks for detailed analysis! It was a hard one to track down :-)

Unfortunately, I don't see how the proposed change fixes the problem.

The failure occurs during SpeciesData instantiation
(BMH$SpeciesData.<init>). It means updateCache has not yet been called
from getForClass:

static SpeciesData getForClass(String types, Class<? extends
BoundMethodHandle> clazz) {
     return updateCache(types, new SpeciesData(types, clazz));
}

With your changes, updateCache is called only after successful loading
of a class. So, the cache isn't updated if there was an error during
previous attempt.

The real problem is more severe IMO. The loaded class is useless when
its initialization finishes abruptly, so there's no way to instantiate
it or reload. Right now it is manifested as a duplicate class definition
attempt, but if you avoid repeated class loading it will just fail
slightly later - the system is already broken and there's no way to recover.

I see the following ways to proceed:
   (1) delay class initialization (fill in Species_*.SPECIES_DATA field
afterwards) and attempt to finish initialization on subsequent requests,
but skipping class loading step;

   (2) use VM anonymous classes (see JDK-8078602 [1]) and just retry
Species creation;

   (3) give up on that particular Species shape and throw an error
whenever it is requested (how it works right now, but with more
meaningful error message).

I remember I experimented with (2), but even after JDK-8078629 [2] there
was still measurable peak performance regression on Octane/Nashorn.

(1) looks more favorable and robust, but right now there shouldn't be
much difference - there's no support in VM to unload MH.linkTo*
adapters, so the failure will repeatedly occur once the code cache is full.

My conclusion is that it is mostly a test problem, but java.lang.invoke
framework should clearly communicate the reason why it fails. Since the
test is aware about code cache overflow problems, it looks preferable to
go with (1) or (3) for now. VM should throw VirtualMachineError on
repeated attempts to instantiate SPECIES_DATA and the test already
filters them out.

Best regards,
Vladimir Ivanov

[1] https://bugs.openjdk.java.net/browse/JDK-8078602
[2] https://bugs.openjdk.java.net/browse/JDK-8078629

On 10/28/15 12:19 PM, Michael Haupt wrote:

    Dear all,

    please review this change.
    Bug: https://bugs.openjdk.java.net/browse/JDK-8131129
    Webrev: http://cr.openjdk.java.net/~mhaupt/8131129/webrev.00/

    Thanks to Peter Levart, who has contributed the actual code to fix
    the issue.

    The background of this is as follows. Bug 8131129 is an intermittent
    failure in a multi-threaded test for LambdaForm caching. The failure
    was not reproducible but used to occur on various platforms every
    now and then. I was able to dig up some stack traces that reveal the
    real cause in internal test logs. These traces finally confirmed a
    suspicion that both Peter and I had had but could, for lack of
    complete failure information, not pin down as the cause of the
    issue. (Most traces from failures of the test were only partial, as
    the test harness is selective about traces it logs.)

    At the heart of the problem is an overflowing code cache for
    adapters in the VM. The test, dynamically generating considerable
    amounts of LambdaForm and BoundMethodHandle species classes, would
    eventually trigger this overflow. The place at which the problem
    would surface is this, in
    BoundMethodHandle.Factory.generateConcreteBMHClass():

    Class<? extends BoundMethodHandle> bmhClass =
          //UNSAFE.defineAnonymousClass(BoundMethodHandle.class,
    classFile, null).asSubclass(BoundMethodHandle.class);
          UNSAFE.defineClass(className, classFile, 0, classFile.length,
                             BoundMethodHandle.class.getClassLoader(), null)
              .asSubclass(BoundMethodHandle.class);
    UNSAFE.ensureClassInitialized(bmhClass);

    The test would generate a BMH species with a given type signature
    via defineClass and immediately thereafter force initialisation of
    that class, in the course of which a SpeciesData instance is
    supposed to be created and entered in the BMH species cache. In case
    the VM's adapter code cache overflowed during this operation, the
    BMH species' class initialiser would not finish its execution
    properly, leading to a stale placeholder entry left behind in the
    cache. The presence of this very placeholder would trick the next
    request for a BMH species with the same type signature into
    believing the class needed to be defined, leading to the observed
    failure.

    An exemplary stack trace is appended below.

    Both Peter and I had come up with solutions, but I think Peter's is
    better as it gets rid of the somewhat intricate and semi-hidden
    cache registration call made from generated code. With Peter's fix,
    it's all visible in library code now.

    Thanks,

    Michael



    -----
    java.lang.InternalError: java.lang.NoSuchMethodException: no such
    method:
    
java.lang.invoke.MethodHandle.linkToStatic(Object,Object,int,Object,Object,Object,Object,Object,Object,Object,MemberName)Object/invokeStatic
             at
    
java.lang.invoke.MethodHandleStatics.newInternalError(MethodHandleStatics.java:120)
             at
    
java.lang.invoke.DirectMethodHandle.makePreparedLambdaForm(DirectMethodHandle.java:214)
             at
    
java.lang.invoke.DirectMethodHandle.preparedLambdaForm(DirectMethodHandle.java:188)
             at
    
java.lang.invoke.DirectMethodHandle.preparedLambdaForm(DirectMethodHandle.java:177)
             at
    java.lang.invoke.DirectMethodHandle.make(DirectMethodHandle.java:84)
             at
    
java.lang.invoke.MethodHandles$Lookup.getDirectMethodCommon(MethodHandles.java:1655)
             at
    
java.lang.invoke.MethodHandles$Lookup.getDirectMethod(MethodHandles.java:1600)
             at
    java.lang.invoke.MethodHandles$Lookup.findStatic(MethodHandles.java:777)
             at
    
java.lang.invoke.BoundMethodHandle$Factory.makeCbmhCtor(BoundMethodHandle.java:817)
             at
    
java.lang.invoke.BoundMethodHandle$Factory.makeCtors(BoundMethodHandle.java:772)
             at
    
java.lang.invoke.BoundMethodHandle$SpeciesData.<init>(BoundMethodHandle.java:347)
             at
    
java.lang.invoke.BoundMethodHandle$SpeciesData.getForClass(BoundMethodHandle.java:411)
             at
    java.lang.invoke.BoundMethodHandle$Species_IL7.<clinit>(Species_IL7)
             at sun.misc.Unsafe.ensureClassInitialized(Native Method)
             at
    
java.lang.invoke.BoundMethodHandle$Factory.generateConcreteBMHClass(BoundMethodHandle.java:718)
             at
    
java.lang.invoke.BoundMethodHandle$SpeciesData.get(BoundMethodHandle.java:401)
             at
    
java.lang.invoke.BoundMethodHandle$SpeciesData.extendWith(BoundMethodHandle.java:388)
             at
    java.lang.invoke.LambdaFormEditor.newSpeciesData(LambdaFormEditor.java:363)
             at
    
java.lang.invoke.LambdaFormEditor.makeArgumentCombinationForm(LambdaFormEditor.java:631)
             at
    
java.lang.invoke.LambdaFormEditor.filterArgumentForm(LambdaFormEditor.java:612)
             at
    java.lang.invoke.MethodHandles.filterArgument(MethodHandles.java:2667)
             at
    java.lang.invoke.MethodHandles.filterArguments(MethodHandles.java:2654)
             at TestMethods$4.getMH(TestMethods.java:180)
             at TestMethods.getTestCaseMH(TestMethods.java:548)
             at
    LFMultiThreadCachingTest.lambda$doTest$0(LFMultiThreadCachingTest.java:80)
             at LFMultiThreadCachingTest$$Lambda$5/21979926.run(Unknown
    Source)
             at java.lang.Thread.run(Thread.java:745)
    Caused by: java.lang.NoSuchMethodException: no such method:
    
java.lang.invoke.MethodHandle.linkToStatic(Object,Object,int,Object,Object,Object,Object,Object,Object,Object,MemberName)Object/invokeStatic
             at
    java.lang.invoke.MemberName.makeAccessException(MemberName.java:873)
             at
    java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:990)
             at
    
java.lang.invoke.DirectMethodHandle.makePreparedLambdaForm(DirectMethodHandle.java:212)
             ... 25 more
    Caused by: java.lang.NoSuchMethodError:
    
java.lang.invoke.MethodHandle.linkToStatic(Ljava/lang/Object;Ljava/lang/Object;ILjava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/invoke/MemberName;)Ljava/lang/Object;
             at java.lang.invoke.MethodHandleNatives.resolve(Native Method)
             at
    java.lang.invoke.MemberName$Factory.resolve(MemberName.java:962)
             at
    java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:987)
             ... 26 more
    Caused by: java.lang.VirtualMachineError: out of space in CodeCache
    for method handle intrinsic
             ... 29 more
    java.lang.LinkageError: loader (instance of  <bootloader>):
    attempted  duplicate class definition for name:
    "java/lang/invoke/BoundMethodHandle$Species_IL7"
             at sun.misc.Unsafe.defineClass(Native Method)
             at
    
java.lang.invoke.BoundMethodHandle$Factory.generateConcreteBMHClass(BoundMethodHandle.java:715)
             at
    
java.lang.invoke.BoundMethodHandle$SpeciesData.get(BoundMethodHandle.java:401)
             at
    
java.lang.invoke.BoundMethodHandle$SpeciesData.extendWith(BoundMethodHandle.java:388)
             at
    java.lang.invoke.LambdaFormEditor.newSpeciesData(LambdaFormEditor.java:363)
             at
    
java.lang.invoke.LambdaFormEditor.makeArgumentCombinationForm(LambdaFormEditor.java:631)
             at
    
java.lang.invoke.LambdaFormEditor.filterArgumentForm(LambdaFormEditor.java:612)
             at
    java.lang.invoke.MethodHandles.filterArgument(MethodHandles.java:2667)
             at
    java.lang.invoke.MethodHandles.filterArguments(MethodHandles.java:2654)
             at TestMethods$4.getMH(TestMethods.java:180)
             at TestMethods.getTestCaseMH(TestMethods.java:548)
             at
    LFMultiThreadCachingTest.lambda$doTest$0(LFMultiThreadCachingTest.java:80)
             at LFMultiThreadCachingTest$$Lambda$5/21979926.run(Unknown
    Source)
             at java.lang.Thread.run(Thread.java:745)
    java.lang.LinkageError: loader (instance of  <bootloader>):
    attempted  duplicate class definition for name:
    "java/lang/invoke/BoundMethodHandle$Species_IL7"
             at sun.misc.Unsafe.defineClass(Native Method)
             at
    
java.lang.invoke.BoundMethodHandle$Factory.generateConcreteBMHClass(BoundMethodHandle.java:715)
             at
    
java.lang.invoke.BoundMethodHandle$SpeciesData.get(BoundMethodHandle.java:401)
             at
    
java.lang.invoke.BoundMethodHandle$SpeciesData.extendWith(BoundMethodHandle.java:388)
             at
    java.lang.invoke.LambdaFormEditor.newSpeciesData(LambdaFormEditor.java:363)
             at
    
java.lang.invoke.LambdaFormEditor.makeArgumentCombinationForm(LambdaFormEditor.java:631)
             at
    
java.lang.invoke.LambdaFormEditor.filterArgumentForm(LambdaFormEditor.java:612)
             at
    java.lang.invoke.MethodHandles.filterArgument(MethodHandles.java:2667)
             at
    java.lang.invoke.MethodHandles.filterArguments(MethodHandles.java:2654)
             at TestMethods$4.getMH(TestMethods.java:180)
             at TestMethods.getTestCaseMH(TestMethods.java:548)
             at
    LFMultiThreadCachingTest.lambda$doTest$0(LFMultiThreadCachingTest.java:80)
             at LFMultiThreadCachingTest$$Lambda$5/21979926.run(Unknown
    Source)
             at java.lang.Thread.run(Thread.java:745)
    STATUS:Failed.STATUS:Failed.STATUS:Failed.`main' threw exception:
    java.lang.LinkageError: loader (instance of <bootloader>): attempted
    duplicate class definition for name:
    "java/lang/invoke/BoundMethodHandle$Species_IL7"
    `main' threw exception: java.lang.LinkageError: loader (instance of
    <bootloader>): attempted duplicate class definition for name:
    "java/lang/invoke/BoundMethodHandle$Species_IL7"
    `main' threw exception: java.lang.InternalError:
    java.lang.NoSuchMethodException: no such method:
    
java.lang.invoke.MethodHandle.linkToStatic(Object,Object,int,Object,Object,Object,Object,Object,Object,Object,MemberName)Object/invokeStatic
    java.lang.LinkageError: loader (instance of  <bootloader>):
    attempted  duplicate class definition for name:
    "java/lang/invoke/BoundMethodHandle$Species_IL7"
             at sun.misc.Unsafe.defineClass(Native Method)
             at
    
java.lang.invoke.BoundMethodHandle$Factory.generateConcreteBMHClass(BoundMethodHandle.java:715)
             at
    
java.lang.invoke.BoundMethodHandle$SpeciesData.get(BoundMethodHandle.java:401)
    -----