Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review how DefaultAnnotationManager processes annotations #3335

Closed
mnriem opened this issue Apr 3, 2023 · 8 comments
Closed

Review how DefaultAnnotationManager processes annotations #3335

mnriem opened this issue Apr 3, 2023 · 8 comments

Comments

@mnriem
Copy link
Contributor

mnriem commented Apr 3, 2023

For background, see eclipse-ee4j/jaxb-ri#1703

@Thihup
Copy link
Collaborator

Thihup commented Apr 4, 2023

From what I recall, we currently do an eager loading of all classes in the artifact.
This can lead to the situations described in the "background", but it also makes a large application take a long time to startup.

As one of the main goals is not to introduce any more dependencies in the core module, I didn't look for any other way to solve it (one way would be using Jandex).
However, as the new Classfile API is being developed, we might use it to parse the class files and only load them if they include some of the annotations we're looking for.

They will include the Classfile API in JDK 21 as a private API but in the future, they plan to release it, so we would still only depend on the Java class library.

@mnriem
Copy link
Contributor Author

mnriem commented Apr 5, 2023

@Thihup Feel free to keep an eye on those developments and hopefully we can then make the default implementation adopt this API once it is available for public use :)

Thihup added a commit to Thihup/piranha that referenced this issue May 9, 2023
@Thihup
Copy link
Collaborator

Thihup commented May 9, 2023

While the API is not public, I used reflection to try it out to see if it would work - and it does!
Thihup@84c2423

The test case mentioned here is now supported by running the application with
JDK_JAVA_OPTIONS="-Dcloud.piranha.extension.annotationscan.experimental.classfile=true" mvn piranha:run (JDK-21 ea)

@mnriem
Copy link
Contributor Author

mnriem commented May 10, 2023

Would it be something we can create as a separate module or do you think that would be overkill?

@Thihup
Copy link
Collaborator

Thihup commented May 11, 2023

@mnriem I think having a separate implementation using the Classfile API would be reasonable. However, it makes its usage more complicated for example running Piranha through the Maven plugin.

@mnriem
Copy link
Contributor Author

mnriem commented May 11, 2023

We can still ship it as part of the regular distribution, but this separates it from a code perspective and is an opt-in so we keep the default implementation clean?

@Thihup
Copy link
Collaborator

Thihup commented May 11, 2023

Do you mean changing the code in the distributions from

context.add(AnnotationScanExtension.class);                 // Annotation scanning

to something like

if (System.getProperty("cloud.piranha.extension.annotationscan.experimental.classfile") != null) {
    context.add(AnnotationScanClassFileExtension.class);   // Annotation scanning using the new Classfile API
} else {
    context.add(AnnotationScanExtension.class);  // Annotation scanning
}

@Thihup
Copy link
Collaborator

Thihup commented May 30, 2023

They will change the Classfile API a little bit before the release of JDK 21, so I'll keep track of the changes and after it has been stabilized, I'll provide a PR.

They plan to include the Classfile API as a preview API in JDK 22. https://bugs.openjdk.org/browse/JDK-8280389

Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 16, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 19, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 19, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 19, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
Thihup added a commit to Thihup/piranha that referenced this issue Jun 20, 2023
@Thihup Thihup closed this as completed in c14803e Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants