JDK-8134329 : TeeOpTest.java fails across platforms after fix for JDK-8129547
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.util.stream
  • Affected Version: 9
  • Priority: P2
  • Status: Closed
  • Resolution: Fixed
  • Submitted: 2015-08-24
  • Updated: 2016-08-24
  • Resolved: 2015-08-25
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 9
9 b80Fixed
Related Reports
Relates :  
JDK-8129547 - Excess entries in BootstrapMethods with the same (bsm, bsmKind, bsmStaticArgs), but different dynamicArgs
Description
After the fix for JDK-8129547 was pushed, the test

java/util/stream/test/org/openjdk/tests/java/util/stream/TeeOpTest.java

fails across platforms.
Comments
Thanks for analysis, Maurizio. Yes, my fix seems to produce a proper BSM call for your testcase: 43: dup 44: invokestatic #4 // Method java/util/Objects.requireNonNull:(Ljava/lang/Object;)Ljava/lang/Object; 47: pop 48: invokedynamic #11, 0 // InvokeDynamic #0:run:(LTest$3;)Ljava/lang/Runnable; <----- OKAY, references m() 53: astore_2 54: return BootstrapMethods: 0: #29 invokestatic java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite; Method arguments: #30 ()V #31 invokevirtual Test.m:()V #30 ()V
25-08-2015
The failure is more nuanced than I was describing - in reality the root cause is that the existing logic was based on the assumption that BSM keys would have same hash/equal properties as their invokedynamic counterparts; under than assumption, the old logic was just fine. But nowe we have a new BSM key class, with different hash/equals properties than that of the invokedynamic entry pointing to it; so we can't safely assume that a new invokedynamic entry will always result in a new BSM key. More specifically, in cases where static aruments are similarly shaped, but dynamic arguments differ, we get a new invokedynamic entry, pointing to an old, shared BSM key. In this case the indexing logic is bogus, as shown by the following code: class Test { static void test() { Runnable r1 = new Test() { }::m; Runnable r2 = new Test() { }::g; Runnable r3 = new Test() { }::m; } void m() { } void g() { } } Which gives (after the fix for 8129547) the following result: static void test(); descriptor: ()V flags: ACC_STATIC Code: stack=2, locals=3, args_size=0 0: new #2 // class Test$1 3: dup 4: invokespecial #3 // Method Test$1."<init>":()V 7: dup 8: invokestatic #4 // Method java/util/Objects.requireNonNull:(Ljava/lang/Object;)Ljava/lang/Object; 11: pop 12: invokedynamic #5, 0 // InvokeDynamic #0:run:(LTest$1;)Ljava/lang/Runnable; 17: astore_0 18: new #6 // class Test$2 21: dup 22: invokespecial #7 // Method Test$2."<init>":()V 25: dup 26: invokestatic #4 // Method java/util/Objects.requireNonNull:(Ljava/lang/Object;)Ljava/lang/Object; 29: pop 30: invokedynamic #8, 0 // InvokeDynamic #1:run:(LTest$2;)Ljava/lang/Runnable; 35: astore_1 36: new #9 // class Test$3 39: dup 40: invokespecial #10 // Method Test$3."<init>":()V 43: dup 44: invokestatic #4 // Method java/util/Objects.requireNonNull:(Ljava/lang/Object;)Ljava/lang/Object; 47: pop 48: invokedynamic #11, 0 // InvokeDynamic #1:run:(LTest$3;)Ljava/lang/Runnable; <-------------------------------------- BOGUS 53: astore_2 54: return Where: BootstrapMethods: 0: #29 invokestatic java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite; Method arguments: #30 ()V #31 invokevirtual Test.m:()V #30 ()V 1: #29 invokestatic java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite; Method arguments: #30 ()V #34 invokevirtual Test.g:()V #30 ()V Fix is to either revert to old behavior, or to go with the new fix by Alex.
25-08-2015
Thanks Maurizio, you're right -- the problem is with BSM indexes. I'd suggest a cleaner fix, see 8134329-poc-fix.patch. It passes the offending TeeOpTest now.
25-08-2015
Seems to me that the bug is here: poolbuf.appendByte(CONSTANT_InvokeDynamic); poolbuf.appendChar(bootstrapMethods.size() - 1); poolbuf.appendChar(pool.put(nameType(dynSym))); ClassWriter.writePool seems to erroneously assume that the indy index in the bsm array should always point to the latest BSM slot - if there are no duplicates this works out ok - but with duplicate this leads to errors - the code should recover whatever index has been associated previously with a duplicate entry and use that instead (where available). In other words: DynamicMethod.BootstrapMethodsKey key = new DynamicMethod.BootstrapMethodsKey(dynSym, types); boolean added = bootstrapMethods.put(key, handle); final int index; if (added) { index = bootstrapMethods.size() -1; } else { int i = 0; for (BootstrapMethodKey key2 : bootstrapMethods.entrySet()) { if (key.equals(key2)) break; i++; } index = key2; }
25-08-2015
I've analysed the differences a bit more - I'm not sure this is a bug with BSM keys - generated code seems correct. All the new version does is removing all the duplicate entries for the 'before'/'after' MHs - that is, the following entries in the original code are merged: 5: #105 invokestatic java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite; Method arguments: #106 (Ljava/lang/Object;)V #127 invokevirtual org/openjdk/tests/java/util/stream/TeeOpTest$AbstractRecordingConsumer.before:(Ljava/util/stream/TestData;)V #128 (Ljava/util/stream/TestData;)V 6: #105 invokestatic java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite; Method arguments: #106 (Ljava/lang/Object;)V #132 invokevirtual org/openjdk/tests/java/util/stream/TeeOpTest$AbstractRecordingConsumer.after:(Ljava/util/stream/TestData;)V #128 (Ljava/util/stream/TestData;)V 8: #105 invokestatic java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite; Method arguments: #106 (Ljava/lang/Object;)V #127 invokevirtual org/openjdk/tests/java/util/stream/TeeOpTest$AbstractRecordingConsumer.before:(Ljava/util/stream/TestData;)V #128 (Ljava/util/stream/TestData;)V 9: #105 invokestatic java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite; Method arguments: #106 (Ljava/lang/Object;)V #132 invokevirtual org/openjdk/tests/java/util/stream/TeeOpTest$AbstractRecordingConsumer.after:(Ljava/util/stream/TestData;)V #128 (Ljava/util/stream/TestData;)V 11: #105 invokestatic java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite; Method arguments: #106 (Ljava/lang/Object;)V #127 invokevirtual org/openjdk/tests/java/util/stream/TeeOpTest$AbstractRecordingConsumer.before:(Ljava/util/stream/TestData;)V #128 (Ljava/util/stream/TestData;)V 12: #105 invokestatic java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite; Method arguments: #106 (Ljava/lang/Object;)V #132 invokevirtual org/openjdk/tests/java/util/stream/TeeOpTest$AbstractRecordingConsumer.after:(Ljava/util/stream/TestData;)V #128 (Ljava/util/stream/TestData;)V So, entries { 5,6 8, 9, 11, 12 } are all identical and are dropped in the new bytecode - this means that we go from 12 to 6 entries, as previously stated. But there's no erroneous merging here. That said - the differences in the indy call themselves look suspicious - for instance this: - 29: invokedynamic #33, 0 // InvokeDynamic #8:accept:(Lorg/openjdk/tests/java/util/stream/TeeOpTest$3RecordingConsumer;)Ljava/util/function/Consumer; + 29: invokedynamic #33, 0 // InvokeDynamic #5:accept:(Lorg/openjdk/tests/java/util/stream/TeeOpTest$3RecordingConsumer;)Ljava/util/function/Consumer; This is odd as #8 (in the old code) and #5 (in the new code) map to completely unrelated BSM entries.
25-08-2015
One thing is not clear - it seems to me that the current equals/hash routines for BSM key do take into account all static arg values; the current logic only special cases those values that happen to be types (i.e. MethodType). But for non-type values, the value is preserved in the uniqueStaticArgs array - and that array is used to perform equality and hash - so it seems like it should work? In the case you mention, the difference seems to involve a method handle - which is an instance of the Pool.MethodHandle class - eyeballing this class it seems like it implements equals/hash correctly as well?
25-08-2015
Hence, I believe the bug is present in javac even before JDK-8129547, since BSM key included both static arg and dynamic arg types before, and we dropped the dynamic arg types. It would seem JDK-8129547 exposed this bug to more conventional cases of similarly-shaped indy calls. It seems TeeOpTest's lambdas differ enough in their dynamic args. This means blindly reverting JDK-8129547 is not a complete solution, and we need to rewire javac's DynamicMethod.BootstrapMethodsKey to handle the static arg *values*.
24-08-2015
Okay, I see the problem: BSMs are apparently getting glued together based on static argument *types*, not the static argument *values*. See: 10: #105 invokestatic <LMF> Method arguments: ... #146 invokestatic org/openjdk/tests/java/util/stream/TeeOpTest.lambda$testDoubleOps$3:(Lorg/openjdk/tests/java/util/stream/TeeOpTest$4RecordingConsumer;Ljava/util/stream/DoubleStream;)Ljava/util/stream/DoubleStream; ... 11: #105 invokestatic <LMF> Method arguments: ... #127 invokevirtual org/openjdk/tests/java/util/stream/TeeOpTest$AbstractRecordingConsumer.before:(Ljava/util/stream/TestData;)V ... 12: #105 invokestatic <LMF> Method arguments: ... #132 invokevirtual org/openjdk/tests/java/util/stream/TeeOpTest$AbstractRecordingConsumer.after:(Ljava/util/stream/TestData;)V ... ...got glued into: 6: #105 invokestatic <LMF> Method arguments: ... #146 invokestatic org/openjdk/tests/java/util/stream/TeeOpTest.lambda$testDoubleOps$3:(Lorg/openjdk/tests/java/util/stream/TeeOpTest$4RecordingConsumer;Ljava/util/stream/DoubleStream;)Ljava/util/stream/DoubleStream; ...
24-08-2015
Ah! The trick to reproduce was to remove JT* files from the last run. jtreg apparently fails us here. Indeed, it seems to fail with JDK-8129547 onboard, and passes with JDK-8129547 reverted. The "javap -c -v" dump yields an expected reduce in BSM count (12 -> 6), and minor expected changes in the bytecode, see javap.diff. Is this a VM bug?
24-08-2015
But... It fails even when JDK-8129547 is reverted.
24-08-2015
Yes, it fails for me on recent jdk9-dev like this: test org.openjdk.tests.java.util.stream.TeeOpTest.testLongOps("LongStream.longRangeClosed(0,l): 100", java.util.stream.TestData$AbstractTestData$LongTestData@5f5be41a): failure java.lang.AbstractMethodError: Method org/openjdk/tests/java/util/stream/TeeOpTest$$Lambda$113.accept(Ljava/lang/Object;)V is abstract at org.openjdk.tests.java.util.stream.TeeOpTest$$Lambda$113/1811134886.accept(Unknown Source) at java.util.stream.OpTestCase$ExerciseDataStreamBuilder.exercise(OpTestCase.java:384) at org.openjdk.tests.java.util.stream.TeeOpTest.testLongOps(TeeOpTest.java:121) at sun.reflect.GeneratedMethodAccessor9.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:519) at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:80) at org.testng.internal.Invoker.invokeMethod(Invoker.java:714) at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:901)
24-08-2015