ArC - get rid of reflection fallback for private injection points

[mkouba]

Currently, we have a reflection fallback for private injection points. This is not ideal. In theory, we could get rid of the reflection fallback if we transform the original class and add a package-private synthetic "setter" method that could be used from the bean metadata class.

public class Foo {
   
   @Inject
   private Bar bar;

   // Generated package-private method - will be called from the bean metadata class
   static set$$bar(Foo foo, Bar bar) {
      foo.bar = bar;
   }
}

The downside of this approach is the cost of transformation.

NOTE: This approach is similar to what the compiler does for nested classes that need to access a private member.

[quarkusbot]

/cc @manovotn

[mkouba]

@manovotn @stuartwdouglas @geoand WDYT?

[stuartwdouglas]

I would like to see some data on how much it would affect hot reload time, startup time, and how much memory it would save.

My gut feeling is that it is not really worth it, but without numbers I am just guessing.

[geoand]

I am pretty sure we already do this in other places, but having it here would be further reaching so it would be nice to have some figures to compare

[mkouba]

@stuartwdouglas I would expect: hot reload time - slightly increase, startup time - no change, native memory - slightly decrease. But it really depends on the app. And I think it's quite tricky to measure. My original motivation was to get rid of the warnings we log for private injection fields. These warnings are probably too strict anyway because many extensions register dozen of classes for reflection...

@geoand Yes, we already remove final from a method when a proxy is required, create a no-args constructor if needed and makes private no-args constructors package-private if necessary.

[manovotn]

I think this is good idea.
We could cobble together a sample with X such injection points but unless that X grows into hundreds, I don't think the difference would be (reliably) measureable wrt to performance. But that's just my personal guess :)

Although overall, it wouldn't affect a user that wants to optimize (e.g. such user still shouldn't use private fields in the first place) and at the same time it would allow greenfield projects (or migrating projects) to code whichever way they want without getting warnings and ultimately giving the upper-hand to native apps. So long as we keep the notion of no using private injection fields as means of optimization, I think this is a good step forward.

[gsmet]

I'm wondering if we should just throw an error in this case now, rather than a warning and have a configuration knob to allow it.

But maybe I'm a bit too radical :).

[mkouba]

But maybe I'm a bit too radical :).

@gsmet There are people who consider private injection points a good practice and they're not going to forgive us ;-).

[geoand]

I agree that it's too harsh of a measure

[arvidvillen]

Any updates on this?
Private field injection would be super nice :)

[mkouba]

Any updates on this?
Private field injection would be super nice :)

Just to be clear - private field injection works fine even now but a reflection-based fallback is used.

[JHahnHRO]

NOTE: This approach is similar to what the compiler does for nested classes that need to access a private member.

Since JEP181 it is no longer necessary for compilers to do that. They could use nests instead. And they should, because that is be more efficient, if I understand the matter correctly (which is far from guaranteed to be honest).

[manovotn]

I've added a possible solution to this in #30278
It by default enables transformation of private injection into package private. If turned off, we still support such injection but use currently existing reflection fallback. Comments are welcome :)

[manovotn]

After some time spent delving into the depths of private field injection, I came up with a conclusion that the solution proposed here (#12597 (comment)) as well as my proposal in #30278 will both have very limited effect on reducing reflection fallback usage overall.

The reason is that we can only perform transformation for actual bean impl classes - meaning a proper class-based bean which has @Inject private Foo foo field.

The following scenarios will still require reflection:

• When a bean class has a superclass that isn't bean (no bean defining annotation or abstract class) and also has private injection point. This can be true for several classes in bean hierarchy,
• Bean a.b.Foo with private IP is extended by another bean x.y.Bar in different package - we can attempt to transform that but will end with scenario below
• Bean a.b.Foo with protected or pack. protected IP is extended by another bean x.y.Bar in different package

Combining any of the above with private IP scenario means we will attempt to perform transformation but will end up registering the class for reflection nonetheless.

We can therefore either accept #30278 as a minimal potential improvement or we can simply close this issue.

EDIT: I have been able to get the related PR to the state where it passes all the tests we are running and have therefore merged it.
With that we are now applying transformation for any private injection point in bean impl class (including interceptors/decorators) that we can find but we still need (and perform) reflection access for cases listed above.