Fix Painless Lambdas for Java 9 #24070

jdconrad · 2017-04-12T15:28:41Z

JDK9 has made changes to LambdaMetaFactory that prevent Painless lambdas from being able to do proper adaptation of types from the interface method to the delegate method even using a bridge. In particular boxed types are no longer widened appropriately, and Object can no longer be adapted to primitive types. Though, the former is likely a bug based on the spec, whether the latter is allowed according to spec is somewhat ambiguous.

This fix adds a LambdaBootstrap class that replaces LambdaMetaFactory in Painless using the same methodology that LambdaMetaFactory was based on (http://cr.openjdk.java.net/~briangoetz/lambda/lambda-translation.html). This allows us to do whatever adaptations we believe are necessary without having to worry about future changes/bugs in LambdaMetaFactory.

This is related to #23473.

and delegate method can be resolved at compile-time.

nik9000 · 2017-04-12T18:05:53Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/Compiler.java

@@ -63,7 +63,7 @@
    }

    /**
-     * A secure class loader used to define Painless scripts.
+     * A secure class loader used to defineScript Painless scripts.


Accidental refactor.

nik9000 · 2017-04-12T18:06:20Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/Compiler.java

    }

    /**
     * Runs the two-pass compiler to generate a Painless script.
     * @param <T> the type of the script
-     * @param loader The ClassLoader used to define the script.
+     * @param loader The ClassLoader used to defineScript the script.


Accidental refactor.

nik9000 · 2017-04-12T18:06:27Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/Compiler.java

            java.lang.reflect.Constructor<? extends PainlessScript> constructor =
                    clazz.getConstructor(String.class, String.class, BitSet.class);

            return iface.cast(constructor.newInstance(name, source, root.getStatements()));
        } catch (Exception exception) { // Catch everything to let the user know this is something caused internally.
-            throw new IllegalStateException("An internal error occurred attempting to define the script [" + name + "].", exception);
+            throw new IllegalStateException("An internal error occurred attempting to defineScript the script [" + name + "].", exception);


Accidental refactor.

nik9000 · 2017-04-12T18:08:28Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/FunctionRef.java

 * <p>
- * Once you have created one of these, you have "everything you need" to call LambdaMetaFactory
- * either statically from bytecode with invokedynamic, or at runtime from Java.  
+ * Once you have created one of these, you have "everything you need" to call LambdaBootstrap


{@link LambdaBootstrap}? The advantage of the @link is that IDEs will automatically fix the comment if you rename the class.

nik9000 · 2017-04-12T18:14:44Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java

+ * lambda functions and method references within Painless.  The code generation
+ * used here is based upon the following article:
+ * http://cr.openjdk.java.net/~briangoetz/lambda/lambda-translation.html
+ * However, it is a simplified version as Painless has no concept of generics.


Nor serialization which the link goes into a lot of depth about.

Also, one of the key reasons that they took that strategy for java code is that the compiler doesn't know what runtime it is going to be run under. invokedynamic future proofs the generated code. We don't have that concern. So I expect we can make different choices there too.

Chatted with @jdconrad - we have a different reason for needing invokedynamic than java does - we don't know the target functional interface until runtime sometimes. If you invoke a method on a def object that takes a lambda then we must delay building the class. For simplicity, we always delay.

Added a note about serialization.

nik9000 · 2017-04-12T18:17:57Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java

+ *             this.arg$0 = arg$0;
+ *         }
+ *
+ *         public static $$Lambda0 get$Lambda(List arg$0) {


Reading this from top to bottom, I'm curious why we need this instead of calling the ctor directly.

To invoke a constructor the class would have to be newed and duped onto the stack. The caller doesn't know what the generated class is, so it calls a factorymethod that can create the class instead.

nik9000 · 2017-04-12T18:21:39Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java

+
+    /**
+     * A counter used to generate a unique name
+     * for each lambda function/reference class.


It is probably worth leaving a comment that this is done in the child classloader so this is a per-script counter.

Looks like that's not the case as when I test with two independent scripts the counter is incremented both times rather than starting at 1 for both of them.

Yeah, there is only a single LambdaBootstrap so there is only one counter. I'd prefer one per script so I could look at the counter and know how many lambdas we've generated but I don't think that is worth holding up the PR for.

It's not worth complicating this to have to reload this class on a per-script basis. If you really want know the lambda count during debugging you can run one script at a time in a debugger. Plus, you also already know the answer since you generate exactly one lambda factory method per lambda per target type.

Right, it isn't worth holding the PR for. I'd like it for debugging issues in running systems rather than the debugger. But it isn't a big deal.

nik9000 · 2017-04-12T18:22:20Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java

+     * @throws LambdaConversionException Thrown when an illegal type conversion occurs at link time
+     */
+    public static CallSite lambdaBootstrap(
+        Lookup lookup,


Can you indent this one extra time so it doesn't look like method body?

jdconrad · 2017-04-14T16:17:41Z

@nik9000 Ready for another round -- think I responded to all comments.

nik9000

I can't say I really understand all the subtleties but I think this makes sense.

I think it'd be worth testing what happens when you blow PIC and this falls back to megamorphic. I'm not sure if this code is worse or better than LambdaMetaFactory in that case but I'd love to understand it.

nik9000 · 2017-04-18T15:45:37Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/Compiler.java

            return defineClass(name, bytes, 0, bytes.length, CODESOURCE).asSubclass(PainlessScript.class);
        }
+
+        Class<?> defineLambda(String name, byte[] bytes) {


What do you think about javadocs for it?

nik9000 · 2017-04-18T17:03:59Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/FunctionRef.java

-            owner = WriterConstants.AUGMENTATION_TYPE.getInternalName();
+
+        if ("<init>".equals(delegateMethod.name)) {
+            delegateInvokeType = Opcodes.H_NEWINVOKESPECIAL;


I'd either statically import all of these opcodes or none of them.

Yes! Fixed.

nik9000 · 2017-04-18T17:11:29Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/node/PSubCallInvoke.java

@@ -33,7 +33,7 @@
 /**
 * Represents a method call.
 */
-final class PSubCallInvoke extends AExpression {
+final class       PSubCallInvoke extends AExpression {


nik9000 · 2017-04-18T17:32:04Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java

+ * {@code
+ * List list1 = new ArrayList(); "
+ * list1.add(2); "
+ * List1 list2 = new ArrayList(); "


s/List1/List/

nik9000 · 2017-04-18T17:45:06Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java

+
+    /**
+     * A counter used to generate a unique name
+     * for each lambda function/reference class.


Yeah, there is only a single LambdaBootstrap so there is only one counter. I'd prefer one per script so I could look at the counter and know how many lambdas we've generated but I don't think that is worth holding up the PR for.

nik9000 · 2017-04-18T17:47:50Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java

+     * of either a lot more code or requiring many {@link Definition.Type}s to be looked
+     * up at link-time.
+     */
+    public static CallSite delegateBootstrap(Lookup lookup,


Can you move this method up with the other public method?

Going to leave this one as the methods are in order of which ones will be called first.

jdconrad · 2017-04-18T19:45:37Z

I don't understand this -- "I think it'd be worth testing what happens when you blow PIC and this falls back to megamorphic. I'm not sure if this code is worse or better than LambdaMetaFactory in that case but I'd love to understand it." Can you please explain what you mean here? Lambdas don't use this PIC at all except indirectly through another def type. A direct lambda cannot be the target type ever. For that def type to even fall back to megamorphic requires the target type to change I believe at least 6 times. (I'm not even sure this is possible using lambdas given our current supported whitelist.) If a user has a def value representing a lambda it's simply thought of as the functional interface type. I also know the fallback is tested for other types already. Maybe it's interesting in an academic sense, but it's really not practically necessary at this time.

nik9000 · 2017-04-18T20:03:41Z

Maybe it's interesting in an academic sense, but it's really not practically necessary at this time.

Sure. I'm debugging some issues someone pinged me about where our stack traces aren't good when we fall back to the megamorphic case so I'm probably overthinking this.

nik9000

Let's get this in so we aren't busted in Java 9.

jdconrad · 2017-04-18T20:16:19Z

@nik9000 Is there an issue up for the stack traces? I'm curious to see what the difference in stack traces looks like.

Also, thanks for the review! I will make sure to respond to the rest of the comments before merging.

nik9000 · 2017-04-18T20:24:35Z

Is there an issue up for the stack traces? I'm curious to see what the difference in stack traces looks like.

I'm playing with it now. Someone pinged my privately. I'm still trying to reproduce.

nik9000 · 2017-04-18T22:56:07Z

So yes, there is an issue with stack traces but it isn't unexpected. Basically the PIC and MIC end up in stacktraces when def is involved and that makes things hard to read. But you should be using script_stack to debug issues like these anyway.

Replaces LambdaMetaFactory with LambdaBootstrap, a custom solution for lambdas in Painless using a design similar to LambdaMetaFactory, but allows for custom adaptation of types which recent changes to LambdaMetaFactory no longer allowed.

jdconrad · 2017-04-24T17:07:35Z

@nik9000 Thank you for the review.

nik9000 · 2017-04-24T17:38:22Z

Thanks for diving into LambdaMetaFactory!

uschindler · 2017-04-29T16:37:50Z

I figured out that the special handling of H_NEWINVOKESPECIAL is broken. The following test fails:

    public void testCtorWithParams() {
        assertArrayEquals(new Object[] { "foo", "bar" },
                (Object[]) exec("List l = new ArrayList(); l.add('foo'); l.add('bar'); " +
                        "Stream stream = l.stream().map(StringBuilder::new);" +
                        "return stream.map(Object::toString).toArray()"));
    }

This code fails because the script returns a Object[] consisting only of 2 empty Strings. The reason for this is: StringBuilder::new points to the StringBuilder(CharSequence) ctor. The code in LambdaBootstrap ignores all parameters and calls the default ctor of StringBuilder. The result is mapped back to a String afterwards and is of course empty.

I have a fix ready...

uschindler · 2017-04-29T17:09:42Z

In addition, the static factory as replacement for the constructor of the lambda (get$lambda) is not needed. If you use lookup.findConstructor() everything works as it should. I will post both fixes in a PR.

uschindler · 2017-04-29T17:38:08Z

I opened PR #24406 to fix both issues. All tests pass for me with Java 8 and Java 9.

uschindler · 2017-04-29T22:27:43Z

I have seen some discussions initiated by @nik9000 and @jdconrad about the useless stack frames we see in the stack trace on exceptions. Since Java 8u60, the whole Lambda class is hidden from all stack traces (so user only sees a direct call in the stack trace to the lambda/function reference, but no wrapper classes).

The trick is to mark methods that you want to hide (because they are completely synthetic and have no useful meaning to users reading the stack). This would also solve the problem with the ever-incrementing class name counter.

This is the change in Java 8u60: http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/rev/f7dd864a52ea

We can do the same (after #24406 is merged):

iface.visitAnnotation("Ljava/lang/invoke/LambdaForm$Hidden;", true);

The problem is that this runtime annotation is internal only (OpenJDK specific). So I'd only add the annotation to the interface method and the ctor proxy only if Class.forName("java.lang.invoke.LambdaForm$Hidden") works. If you agree, I will provide a PR soon after #24406 is merged.

uschindler · 2017-04-29T23:57:59Z

I tried to hide the stack frames like done above. Unfortunately it does not seem to work if the classes are not anonymous (via Unsafe).

During that I found some smaller things, like the doPrivileged around getting the ctor: This is not needed, as it is public in our case. I can provide a PR later. I'd also change LambdaConversionException like the original LambdaMetaFactory on problems instead of IllegalStateException (which is wrong).

uschindler · 2017-04-30T09:23:48Z

As hiding the stack frames did not work, I made the counter "per-script". The trick was to put an AtomicInteger into the Compiler.Loader instance, which is accessible from LambdaBootstrap. As it is no longer global, an integer is fine. I also removed the doPrivileged for the non-capturing instance. See b547fa9

I don't like the IllegalStateExceptions in LambdaBootstrap instead of LambdaConversionExceptions. We should also change that. OpenJDKs bootstrap throws LambdaConversionException on every problem while creating the lambda class.

uschindler · 2017-04-30T09:45:07Z

Finally I also changed the codesource parameter in defineClass to be consistent in my PR. The lambdas are now running in same unprivileged code source.

uschindler

I solved the mentioned issues in my other PR #24406

uschindler · 2017-04-28T21:48:11Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/LambdaBootstrap.java

+        // A new instance of the requested type will be created and the
+        // constructor with no parameters will be called.
+        // Example: String::new
+        if (delegateInvokeType == H_NEWINVOKESPECIAL) {


Are we sure here that we never have arguments for ctor references? If no (e.g., painless does not support ::new with args) we should assert this and throw exception!

uschindler · 2017-04-28T22:17:08Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/Compiler.java

+         * @return A Class object.
+         */
+        Class<?> defineLambda(String name, byte[] bytes) {
+            return defineClass(name, bytes, 0, bytes.length);


Why not here also use CODESOURCE for safety? I know the lambda class on its own cannot do any bad stuff, but I'd still make it use the same codesource!

jdconrad added 13 commits March 28, 2017 16:26

Get lambdas working.

35e04b2

Prototype with all indy instructions.

94e2466

Fix lambdas.

b8b1d31

Fix references.

d4b6e06

Fix augmentation used for function refs.

fca2c12

Fix augmentation.

1027122

Start of clean up.

0e58250

More clean up.

7f37bc2

Add documentation to LambdaBootstrap.

73cdba5

Clean up FunctionRef.

57ab5fa

Cleand up tests. Added appropriate cast checking when both the inteface

f442c7c

and delegate method can be resolved at compile-time.

Merge branch 'master' into lambda

0238b31

Merge branch 'master' into lambda

e4c8c34

jdconrad added :Plugin Lang Painless v6.0.0-alpha1 >bug jvm bug jdk9 labels Apr 12, 2017

nik9000 reviewed Apr 12, 2017

View reviewed changes

jdconrad added 2 commits April 14, 2017 09:07

Responded to PR comments.

d4b8b02

Merge branch 'master' into lambda

00e7133

nik9000 mentioned this pull request Apr 18, 2017

Start on custom whitelists for Painless #23563

Merged

nik9000 reviewed Apr 18, 2017

View reviewed changes

nik9000 approved these changes Apr 18, 2017

View reviewed changes

jdconrad added the v5.5.0 label Apr 24, 2017

jdconrad added 4 commits April 24, 2017 09:27

Minor style fixes.

aada4e0

Merge branch 'master' into lambda

6e54219

Merge branch 'master' into lambda

d5716b1

Test fixes.

0c8a9d1

jdconrad merged commit 30cc33e into elastic:master Apr 24, 2017

uschindler mentioned this pull request Apr 29, 2017

painless: Fix method references to ctor with the new LambdaBootstrap and cleanup code #24406

Merged

uschindler reviewed Apr 30, 2017

View reviewed changes

jdconrad deleted the lambda branch May 11, 2017 16:27

clintongormley added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache and removed :Plugin Lang Painless labels Feb 14, 2018

Fix Painless Lambdas for Java 9 #24070

Fix Painless Lambdas for Java 9 #24070

Conversation

jdconrad commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdconrad Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdconrad commented Apr 14, 2017

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdconrad commented Apr 18, 2017 • edited Loading

nik9000 commented Apr 18, 2017

nik9000 left a comment

Choose a reason for hiding this comment

jdconrad commented Apr 18, 2017

nik9000 commented Apr 18, 2017

nik9000 commented Apr 18, 2017

jdconrad commented Apr 24, 2017

nik9000 commented Apr 24, 2017

uschindler commented Apr 29, 2017 • edited Loading

uschindler commented Apr 29, 2017 • edited Loading

uschindler commented Apr 29, 2017

uschindler commented Apr 29, 2017 • edited Loading

uschindler commented Apr 29, 2017

uschindler commented Apr 30, 2017 • edited Loading

uschindler commented Apr 30, 2017

uschindler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdconrad Apr 18, 2017 •

edited

Loading

jdconrad commented Apr 18, 2017 •

edited

Loading

uschindler commented Apr 29, 2017 •

edited

Loading

uschindler commented Apr 29, 2017 •

edited

Loading

uschindler commented Apr 29, 2017 •

edited

Loading

uschindler commented Apr 30, 2017 •

edited

Loading