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.

Sign up for GitHub

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

Jump to bottom

Don't generate BeanDefinitionReference and BeanIntrospectionReference #10490

Merged
merged 10 commits into from
Feb 21, 2024
Merged

Don't generate BeanDefinitionReference and BeanIntrospectionReference #10490

merged 10 commits into from
Feb 21, 2024

Conversation

dstepanov
Copy link
Contributor

@dstepanov dstepanov commented Feb 13, 2024 •
edited
Loading

I don't see any benefit to generating BeanDefinitionReference and BeanIntrospectionReference, but there are some benefits now:

  • Generating fewer classes should speed up the compilation
  • Probably less JVM overhead
  • BDs are now initialized at the build time for the native image
  • Executable is smaller :test-suite-http-server-tck-netty:nativeTestCompile 77->73.7 MB

@dstepanov dstepanov added the type: improvement A minor improvement to an existing feature label Feb 13, 2024
@dstepanov dstepanov marked this pull request as draft February 13, 2024 14:31
@dstepanov
Copy link
Contributor Author

Native size.
Before:
Screenshot 2024-02-14 at 15 19 02
After:
Screenshot 2024-02-14 at 15 18 53
Somehow Serde size got smaller even, so the changes are not projected there

@dstepanov dstepanov changed the title Don't generate BeanDefinitionReference Don't generate BeanDefinitionReference and BeanIntrospectionReference Feb 15, 2024
@dstepanov dstepanov marked this pull request as ready for review February 19, 2024 16:15
@dstepanov
Copy link
Contributor Author

dstepanov commented Feb 19, 2024 •
edited
Loading

I verified that it works with Micronaut Serialization, and I ran some Micronaut Data tests.

yawkat
yawkat approved these changes Feb 20, 2024
View reviewed changes
Copy link
Member

@yawkat yawkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a good change but a bit risky

core-processor/src/main/java/io/micronaut/inject/beans/visitor/BeanIntrospectionWriter.java Show resolved Hide resolved
inject/src/main/java/io/micronaut/context/AbstractInitializableBeanDefinition.java Outdated Show resolved Hide resolved
inject/src/main/java/io/micronaut/context/DefaultBeanContext.java Outdated Show resolved Hide resolved
inject/src/main/java/io/micronaut/context/RequiresCondition.java Outdated Show resolved Hide resolved
@graemerocher
Copy link
Contributor

What is the binary compatibility story here? Will it break binary compatibility and require rebuilds of all modules?

@dstepanov
Copy link
Contributor Author

It is binary-compatible with the previously compiled references and bean definitions.

@dstepanov
Copy link
Contributor Author

Added abstract classes

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
7 New issues

Measures
0 Security Hotspots
91.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@dstepanov dstepanov merged commit 8c12ad3 into 4.4.x Feb 21, 2024
15 checks passed
@dstepanov dstepanov deleted the bd branch February 21, 2024 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yawkat yawkat yawkat approved these changes

@graemerocher graemerocher graemerocher approved these changes

Assignees
No one assigned
Labels
type: improvement A minor improvement to an existing feature
Projects
4.4.0 Release
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dstepanov @graemerocher @yawkat