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

[uschindler]

This PR fixes #24070 (comment):

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.

To fix this I changed the LamdaBootstrap to create a static proxy/delegator method around the ctor (delegate$ctor) with the same parameters as the ctor, returning the newly created instance (somehow similar like the factory for the captured lambda - which I removed, see below). When generating the INVOKEDYNAMIC to our type-adapting callsite, we change the H_NEWINVOKESPECIAL to a static method call to our proxy. I tried to fix this without using a static method, but although the JLS/class file format allows H_NEWINVOKESPECIAL for a handle, when passing this as a Handle (in constant pool) to INVOKEDYNAMIC, it causes a class format error. I am not sure if this is a bug in ASM or if INVOKEDYNAMIC does not support this. It has nothing to do with the fact of a newinstance bytecode may be needed - no it's not for MethodHandles!!!

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. So I also removed it. @jdconrad said (#24070 (comment)) that its needed, but it isn't, because lookup.findConstructor() returns a MethodHandle that also does newinstance bytecode.

Documentation was updated accordingly.

The final TODO is to check why the direct MH in constant pool leads to a ClassFormatError. I will look into lambdas created by javac, maybe I see something that I was missing!

[elasticmachine]

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

[uschindler]

I cleaned up the code a bit more and removed most of the IMHO "bad" conversions between internal/slashed class names and binary class names. Instead I propagated the ASM Type, whcih also makes it type safe. This also removes duplicated method calls.

[uschindler]

I checked the bytecode of a javac-compiled class similar to the failing test and figured out with javap:

BootstrapMethods:
  0: #72 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:
      #74 (Ljava/lang/Object;)Ljava/lang/Object;
      #80 newinvokespecial java/lang/StringBuilder."<init>":(Ljava/lang/CharSequence;)V
      #82 (Ljava/lang/CharSequence;)Ljava/lang/StringBuilder;

As we see the original LambdaMetaFactory takes the last parameter with the "real signature" (#82) of the MethodHandle. The Handle in #80 is exactly as I would expected the ctor to be referred to. In fact LambdaMetaFactory does adapt this like we do: It converts the NEWINVOKESPECIAL into a sequence of a newinstance followed by an invokespecial of the ctor using the void return type.

My code does exactly the same type of adaption (in a static helper method calling the constructor after creating the instance). In our case we just use a static helper method, so we can delegate the whole thing easily to delegateBootstrap(). For all other invocation types we do not need any special adaption.

[uschindler]

I think this is ready now. I moved some code around and it looks elegant to me! 🥇

[uschindler]

I also moved the lambda counter to the classloader so it is per script (makes debugging easier and results more consistent). I also removed the doPrivileged for the constructor, as its not needed (OpenJDK code does it, because it uses setAccessible, because lambda class is package private).

[uschindler]

I also changed the code source of the lambda classes to be the same unprivileged as the script itsself.

[jdconrad]

@uschindler I can't thank you enough for fixing all these things! I really appreciate you going through all my code again :). I'll get this checked in today or tomorrow.

On the constructors with arguments -- just a really poor assumption on my part that all constructors using ::new would have no parameters, I'm not sure why I was thinking that way.
On the counter -- you provided a much better design, I just hadn't thought of doing it this way.
On the CODESOURCE -- same answer as just above.
On the factory and lookup.findStatic -- I think I was trying to imitate what LambdaMetaFactory was doing, but I was just wrong about not being able to call the constructor directly. I guess my question here is, is there a reason that LambdaMetaFactory didn't use lookup.findConstructor here?

[uschindler]

On the constructors with arguments -- just a really poor assumption on my part that all constructors using ::new would have no parameters, I'm not sure why I was thinking that way.

No problem. There was also no test about this!

Anyhow, the idea you had and how to use invokedynamic to call asType() to do the type adaption is really phantastic! Incredible cool! Congrats!

I spent nights to think about how to do this without lots of code redoing the same thing like asType! And on top, this causes no slowdown, as the invokedyanmic is done once and then reused (its a constant callsite).

On the factory and lookup.findStatic -- I think I was trying to imitate what LambdaMetaFactory was doing, but I was just wrong about not being able to call the constructor directly. I guess my question here is, is there a reason that LambdaMetaFactory didn't use lookup.findConstructor here?

Very simple reason, they can't do otherwise! The original LambdaMetaFactory has some bootstrapping issues. Because of this it does a lot of stuff only using internal APIs only and doesn't touch public stuff (which is not yet initialized during JVM bootstrapping). In addition, this has less security checks than the public APIs, which prevents them from doing doPrivileged everywhere! E.g. it uses a limited, private-only lookup instance (implLookup, which is a early stage lookup without any security features) and it also creates method handles using that (e.g. the one to the static factory). It also uses Unsafe to define anonymous classes! Because of this it also has to handle security in a very different way, so never ever copy the code from inside the original LambdaMetaFactory. The reason for all this is: While bootstrapping the JVM, they already use lambdas and so there is some chicken and egg problems. Not everything is initialized, so the public APIs may break. Because of this they limit internally to private APIs. And the ctor features need more bootstrapping. Our code uses public APIs and so it can directly return a MethodHandle to call the lambda's class' ctor (internally this calls "newinstance" followed up by an "invokespecial", like your static method). In general, it is a bad idea to look at internal OpenJDK implementations and try to reimplement them in "user code" (it should also not done because of GPL-related issues!). I know that you copypasted some code, because you added the no-op doPrivileged with the same try-catch-structure as the GPLed code. The patch here removes all this stuff that somebody could use to argue that ES was copying GPL code!

[jdconrad]

@uschindler Thank you for the explanation here, I think it's clear that I don't deal with the internal APIs in this way very often :) so the chicken and egg problems related to bootstrapping never really occurred to me until you pointed it out. Also I greatly appreciate the help in avoiding any licensing issues.

I'm also happy you approved of using invokedynamic to get access to asType for adaptation. I was worried this wasn't a good way to do the adaptation, but I really didn't want to re-write adaptation code.

[uschindler]

Hi, another reason why the original JDK8 code used a static factory was that ctor MethodHandles were not direct. Since Java 8u40 the new LambdaForms made this obsolete, too: the constructor MHs are direct and are as effective as a calls to static methods. The subclass DirectMethodHandle.Constructor just injects an Unsafe.allocateInstance() before invokespecial to the original constructor (identical to the static factory). So the static factory is just a relic from earlier days where performance was an issue.

[jdconrad]

@elasticmachine test this please

[jdconrad]

@elasticmachine test this please

[uschindler]

Does Gradle not show too long lines when running tests? I thought it did in the past...!

[jdconrad]

gradle check should check all the style as far as I know. When I ran locally gradle check failed for me, but gradle test passed. A lot of this stuff has changed over the past year or two, so it's definitely possible that gradle test used to check line length as well. I blame @rjernst :)

[uschindler]

Ok, will check next time.

BTW: I hate line length limitations. The code is much less readable to me with enforced breaks. 🤐

[jdconrad]

No worries! And I agree with you on the length limits, but it's not up to me.

[jdconrad]

Thanks again @uschindler for all the help with this! I greatly appreciate your expertise in getting this all working correctly. This was committed to both master and 5.x

[uschindler]

OK. All fine :-) Thanks for committing. I opened another PR for adding two Java 9 related tests 2 days ago! #24405

[jdconrad]

Oops, sorry I missed those. I'll take a look right now.