Lazily perform reflection when Instance<> is used

[geoand]

The idea here is to save a few reflection calls
that might never end up being used (this is the
case with some Quarkus REST providers)

For example io.quarkus.resteasy.reactive.jackson.runtime.serialisers.FullyFeaturedServerJacksonMessageBodyReader
has the following constructor:

    public FullyFeaturedServerJacksonMessageBodyReader(Instance<ObjectMapper> mapper, Providers providers) {
        super(mapper);
        this.originalMapper = mapper;
        this.providers = providers;
    }

With the existing version of the code the generated FullyFeaturedServerJacksonMessageBodyReader_Bean looks (partly) like this:

      Constructor var12 = Reflections.findConstructor(FullyFeaturedServerJacksonMessageBodyReader.class, var8);
      InstanceProvider var13 = new InstanceProvider((Type)var9, var10, this, var11, var12, 0, false);
      FixedValueSupplier var14 = new FixedValueSupplier(var13);

With this change it becomes:

      Supplier var12 = Reflections.findConstructorLazily(FullyFeaturedServerJacksonMessageBodyReader.class, var8);
      InstanceProvider var13 = new InstanceProvider((Type)var9, var10, this, var11, var12, 0, false);
      FixedValueSupplier var14 = new FixedValueSupplier(var13);

This means that if FullyFeaturedServerJacksonMessageBodyReader is never used (which is most of the time), no reflective call will be made to look up its constructor

P.S. The change does the minimum possible to achieve what I had in mind,
it definitely is not polished in any way.

[geoand]

I realized that I want to do similar things to make Instance not result in almost any reflection / class loading so I can avoid the eagerly:

      ClassLoader var1 = Thread.currentThread().getContextClassLoader();
      Class var3 = Class.forName("com.fasterxml.jackson.databind.ObjectMapper", false, var1);

[geoand]

The latest commit changes things up and essentially moves stuff out of the constructor into a purposely generated supplier.
This achieves what I want (i.e. not loading any necessary classes or performing any reflection until the bean is actually needed) for the Jackson related JAX-RS / Jakarta REST components.

However this doesn't completely solve the problem of not loading ObjectMapper as a class until needed it just kicks the can down the road.
We now have the following:

I could envision expanding the method used here to other places in the Arc code where FixedValueSupplier is used (like the 4th commit). WDYT?

[geoand]

Question: Can get be called multiple times on these suppliers? If so, then I need to guard against it implementing the get method and having the generated logic in a doGet or something.

[geoand]

@mkouba @manovotn @Ladicek WDYT about this approach?

[mkouba]

I have a few silly questions ;-).

FullyFeaturedServerJacksonMessageBodyReader does not define a scope, so it's @Dependent (because it's added by the extension as an additional bean). This means that every time you obtain/inject the FullyFeaturedServerJacksonMessageBodyReader a new instance is created. Now the question is - how exactly does resteasy reactive handle the readers? Are they cached per resource, or?

Is there some static init performed by com.fasterxml.jackson.databind.ObjectMapper you want to avoid? What's the exact reason? I mean, the ObjectMapper will be used in most cases anyway, or?

BTW I cannot download the flamegraph from #45276 (comment) - it seems to be a private image.

[geoand]

FullyFeaturedServerJacksonMessageBodyReader does not define a scope, so it's @dependent (because it's added by the extension as an additional bean). This means that every time you obtain/inject the FullyFeaturedServerJacksonMessageBodyReader a new instance is created. Now the question is - how exactly does resteasy reactive handle the readers? Are they cached per resource, or?

This is an excellent question. It should just be @ApplicationScoped bean. That wouldn't change anything WRT to proposed change though, right?

Is there some static init performed by com.fasterxml.jackson.databind.ObjectMapper you want to avoid? What's the exact reason? I mean, the ObjectMapper will be used in most cases anyway, or?

I want to avoid loading the class altogether if possible, until this class is actually used. But if that is not possible (which given that we probably need types, I assume is the case), at the very least I want to avoid all the reflective calls since it's entirely possible that this metadata will never be needed.

BTW I cannot download the flamegraph from #45276 (comment) - it seems to be a private image.

I can produce this for you, or give you instructions on how to produce it on your own if you like

[mkouba]

This is an excellent question. It should just be @ApplicationScoped bean. That wouldn't change anything WRT to proposed change though, right?

Right, it would not change anything. I was just trying to understand the broader RR context.

I want to avoid loading the class altogether if possible, until this class is actually used. But if that is not possible (which given that we probably need types, I assume is the case), at the very least I want to avoid all the reflective calls since it's entirely possible that this metadata will never be needed.

Understood, but is one constructor lookup really worth the effort?

[geoand]

Understood, but is one constructor lookup really worth the effort?

That's what I'm trying to understand. I would like to understand what these generated classes actually do and whether the metadata they pull in their constructor is needed when the bean is created, or if it is needed only to support some small part of the Arc/CDI API.
If it's the latter, then avoiding doing reflection on a ton of beans makes sense, if it's the former, then indeed it doesn't make a ton of sense.

[mkouba]

Understood, but is one constructor lookup really worth the effort?

That's what I'm trying to understand. I would like to understand what these generated classes actually do and whether the metadata they pull in their constructor is needed when the bean is created, or if it is needed only to support some small part of the Arc/CDI API. If it's the latter, then avoiding doing reflection on a ton of beans makes sense, if it's the former, then indeed it doesn't make a ton of sense.

So in this particular case the modification that matters is in the BuiltinBean#generateInstanceBytecode(GeneratorContext) method which is called when an injection point resolves to the jakarta.enterprise.inject.Instance built-in bean. And it does not really matter if the injection point represents a field, a method param, or constructor param.

We need the java member (field, constructor, etc.) in order to support injection of InjectionPoint metadata in the bean instances obtained programmatically via an injected Instance (in this case, the @Inject InjectionPoint declared on the @Dependent bean corresponds to the @Inject Instance injection point).

One thing that we could do (and probably should do ;-) is to only collect the injection point metadata when needed, i.e. find all beans that match the given @Inject Instance and simply skip if there's no @Dependent bean with @Inject InjectionPoint. This should also solve the FullyFeaturedServerJacksonMessageBodyReader problem unless there is a custom @Dependent bean for ObjectMapper... let me try to sketch something quickly.

[geoand]

🙏🏽

[geoand]

@mkouba would you be so kind to explain to me why for example the generated _Bean class for BasicAuthenticationMechanism_Bean has to setup an InjectPointImpl (which requires doing reflection to get the member)?

[mkouba]

@mkouba would you be so kind to explain to me why for example the generated _Bean class for BasicAuthenticationMechanism_Bean has to setup an InjectPointImpl (which requires doing reflection to get the member)?

Hm, it's because it injects the HttpConfiguration and HttpBuildTimeConfig for which we create synthetic @Dependent beans and because we don't really know if IP metadata will be needed (well, we do if synthetic injection points are used...) for a synthetic bean we always set the current injection point (which requires reflection). We could be more strict and require synthetic injection points for InjectionPoint metadata. It's a super simple change. Let me send a PR and we'll see if it breaks anything.

BTW this change would not help for @ConfigMappings for which we add a synthetic IP here: https://github.com/quarkusio/quarkus/blob/main/extensions/arc/deployment/src/main/java/io/quarkus/arc/deployment/ConfigBuildStep.java#L339 (it's needed in the io.quarkus.arc.runtime.ConfigMappingCreator)

[geoand]

🙏🏽