Support newInstance() without reflection configuration when extracted to a method

[sdeleuze]

As a kind of follow-up of the idea I had when creating #905 (cc @cstancu) and of our recent discussions about how to improve GraalVM native static analysis to require less reflection configuration (important for Spring but not only), I think I have found an area where GraalVM native could be improved.

The repro is pretty simple, consider:

interface Foo {
	String foo();
}

public class FooImpl implements Foo {
	@Override
	public String foo() {
		return "foo";
	}
}

If I try to instantiate FooImpl via FooImpl.class.getDeclaredConstructor().newInstance(), the application compiles without reflection configuration needed:

public class App {

    public static void main(String[] args) throws NoSuchMethodException, IllegalAccessException, InvocationTargetException, InstantiationException {
        Foo foo = FooImpl.class.getDeclaredConstructor().newInstance();
        System.out.println(foo.foo());
    }

If I just extract the instantiation logic to a method (that's what we do with org.springframework.beans.BeanUtils#instantiateClass(java.lang.Class<T>) that is used everywhere in Spring) like that:

public class App {

    public static void main(String[] args) {
        Foo foo = instantiateClass(FooImpl.class);
        System.out.println(foo.foo());
    }

    public static <T> T instantiateClass(Class<T> clazz) {
        try {
            return clazz.getDeclaredConstructor().newInstance();
        }
        catch (Exception e) {
            e.printStackTrace();
        }
        return null;
    }
}

Then running the native image fails with java.lang.NoSuchMethodException: com.sample.FooImpl.<init>() and requires reflection to work as expected. Same behavior if I use org.springframework.beans.BeanUtils#instantiateClass(java.lang.Class<T>).

Could you improve GraalVM native in order to support this kind of use case without requiring reflection configuration? I think that could allow us to support most function Spring application with almost no reflection configuration and would lead to smaller images that would consume less memory.

Repro project: https://github.com/sdeleuze/graal-issues/tree/master/reflection-static-analysis
Tested with GraalVM 20.1.0

Please tag this issue with spring label.

[sdeleuze]

Any feedback on how hard it could be to support that? Could you please tag it with spring label?

[christianwimmer]

We currently have a research project that looks at method inlining before static analysis. We are starting with simpler cases that do not involve method calls, but then want to expand it to also cover cases like this.

[sdeleuze]

I have worked on this issue related to concrete Spring use cases, and would like to share a few interesting findings.

We have 2 variants of GenericApplicationContext#registerBean which are used to instantiate beans in a programmatic way:

• context.registerBean(Foo.class) currently does not work without reflection configuration (hence this issue)
• context.registerBean(Foo.class, Foo::new) or context.registerBean(Foo.class, () -> new Foo()) work out of the box without any configuration.

The good news is that we should be able to move forward short term with the second variant in order to experiment on removing the need for native-image reflection configuration via functional bean registration.

At the same time, we still need this issue to be fixed because using context.registerBean(Foo.class, Foo::new) just to help native-image compiler is an artificial constraint and more importantly in various places the lambda variant is not possible, like in DispatcherServlet class which is at the heart of Spring MVC.

I have created a functional bean registration repro project here to provide a realistic use case that we hope native-image compiler will support in the future without requiring any configuration (this is a more realistic variant of the initial repro).

[vjovanov]

We have a prototype that could solve this but it is still early to merge it in. I am moving this issue for 20.3.

[vjovanov]

This should be addressed by the following commit:

17f1de4

Feel free to try this out by using -H:+InlineBeforeAnalysis.

[marjanasolajic]

The native-image option -H:+InlineBeforeAnalysis can be used (for now) without requiring reflection configuration for this example:

public static void main(String[] args) throws NoSuchMethodException, IllegalAccessException, InvocationTargetException, InstantiationException {
    Constructor<FooImpl> fooConstructor = getDeclaredConstructor(FooImpl.class);
    Foo foo = fooConstructor.newInstance();
    System.out.println(foo.foo());
    Class<? extends Foo> fooClass = foo.getClass();
    System.out.println("Class name new: " + fooClass.getName());
}

public static <T> Constructor<T> getDeclaredConstructor(Class<T> clazz) throws NoSuchMethodException {
    return clazz.getDeclaredConstructor();
}

[sdeleuze]

Thanks for those progresses that indeed now support use cases where the return value is a constant.

As expected Spring use cases where that would be useful like reflection-static-analysis and reflection-static-analysis-functional are not yet supported because the return value is a new instance, not a constant.

Could we reopen this issue (and removed the 21.0 target version) and start a discussion about how the use case below could be supported:

public class App {

    public static void main(String[] args) {
        Foo foo = instantiateClass(FooImpl.class);
        System.out.println(foo.foo());
    }

    public static <T> T instantiateClass(Class<T> clazz) {
        try {
            return clazz.getDeclaredConstructor().newInstance();
        }
        catch (Exception e) {
            e.printStackTrace();
        }
        return null;
    }
}

I guess the main question is what kind of general pattern could we use to make that use case recognized by -H:+InlineBeforeAnalysis. Any thoughts @marjanasolajic @vjovanov ?

[christianwimmer]

For the more complex use cases like your instantiateClass, we need a deeper redesign of the native image generator. Even if the call to getDeclaredConstructor gets constant folded, there are still two calls remaining: the newInstance and the printStackTrace (or any better way of exception handling). Currently we parse bytecodes twice (for analysis and for AOT compilation), and information is transported between the two parsings using the bci as the key. With two invokes in the callee, there is no more unique bci key in the caller.

I am currently thinking about a redesign of the native image generator that parses bytecodes only once. But that will be a few months before it can be finished. The main takeaway of the InlineBeforeAnalysis work is that every "work around solution" is quite fragile and will always have limitations, and that the redesign is really the only feasible option.

[sdeleuze]

@christianwimmer Thanks for sharing more details, could you please remove the 21.0 target for that issue? Should this issue be reopened or do you prefer to create a new one related to the redesign you mentioned?

[christianwimmer]

@sdeleuze I merged an updated version of "inlining before analysis" last week. This is now the future-proof version that we want to enable by default. But for the upcoming 21.2 release it remains behind the InlineBeforeAnalysis option as before.

I looked at what the new approach is doing with the instantiateClass method from Spring: https://github.com/spring-projects/spring-framework/blob/main/spring-beans/src/main/java/org/springframework/beans/BeanUtils.java#L141
The good news is that we can get these kind of methods working. But there need to be small refactorings. In particular, in the

try {
    return instantiateClass(clazz.getDeclaredConstructor());
} catch ...

it is problematic that the instantiateClass is within the try block. Because even when the getDeclaredConstructor is folded to a constant, the whole complicated catch blocks still remain reachable and therefore the method is way too big to ever be inlined.

When changing the method to

        Assert.notNull(clazz, "Class must not be null");
        if (clazz.isInterface()) {
            throw new BeanInstantiationException(clazz, "Specified class is an interface");
        }
        Constructor<T> ctor;
        try {
            ctor = clazz.getDeclaredConstructor();
        } catch (NoSuchMethodException ex) {
            ctor = findPrimaryConstructor(clazz);
            if (ctor == null) {
                throw new BeanInstantiationException(clazz, "No default constructor found", ex);
            }
        } catch (LinkageError err) {
            throw new BeanInstantiationException(clazz, "Unresolvable class definition", err);
        }
        return instantiateClass(ctor);

it can be inlined when the arguments are constants, because everything apart from the instantiateClass folds away.

This is of course a change with a semantic difference: a LinkageError being thrown by the constructor is no longer converted into a BeanInstantiationException. But if that was necessary, then the instantiateClass(Constructor<T> ctor, Object... args) method would actually need to catch LinkageError itself I guess.

Another barrier for inlining is class initialization. As it is written right now, BeanUtils cannot be initialized at image build time automatically. And the logger that is stored in a static final field probably also prevents you from manually initializing the class at build time. The solution for that would be to put the static final fields into an inner class. That way only the methods that really perform logging would trigger initialization of the logger.

KotlinDetector is probably another class that you want initialized at image build time. Because then all the "is Kotlin present" checks would fold away.

When Kotlin is really present, of course many lookups will not constant fold for the Kotlin part. I have not followed in detail where KotlinDelegate.findPrimaryConstructor goes to - but I can hardly think it leads to something that can be constant folded without a special Native Image plugin that intercepts reflection lookups, like we also do for Class.getDeclaredConstructor.

[sdeleuze]

@christianwimmer Thanks for working on that.

• We will do the refactoring for LinkageError via Allow BeanUtils#instantiateClass inlining with native spring-projects/spring-framework#27072.
• The logging one via Remove logging dependency in BeanUtils spring-projects/spring-framework#27070.
• KotlinDetector is already initialized at build time.
• Kotlin runtime reflection is kind of crazy, my goal is just to move it to AOT, see Optimize Kotlin reflection runtime efficiency spring-projects/spring-framework#21546. Just not sure yet when it will be done.

[christianwimmer]

@sdeleuze Sounds good, let me know if you need help when you experiment with it. Also we probably want some kind of tracing so that users like you know what gets folded and what not. Especially the latter is difficult, because we can certainly not just print all method invokes that are not folded. So there probably needs to be a filtering.

[sdeleuze]

@christianwimmer Should the milestone be updated to 21.2 instead 21.0?

[christianwimmer]

I changed the version to 21.3 since this will be the version where it is enabled by default.

[sdeleuze]

I was able to test it works as expected with spring-attic/spring-native@547d5dd, thanks.