-
Notifications
You must be signed in to change notification settings - Fork 3k
Bytecode transformers - Optimize constant pool scanning optimization #49533
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
Bytecode transformers - Optimize constant pool scanning optimization #49533
Conversation
In BytecodeTransformerBuildItem, we can define a set of classes which will be used to determine if a class is affected by a transformation by by checking the constant pool. This is most notably (and I think exclusively) used as an optimization for the Panache transformation of public fields -> accessors. When you have an application with a lot of entities, all the BytecodeTransformerBuildItem targeting the classes using these entities are carrying the Set of all the entities. And we used copy the Set over and over for each class affected by the transformation, which was generating a lot of useless allocations. Obviously, this optimization only works if we don't use the constant pool scanning optimization for other transformations. And that why, I ended up adding another optimization: we now only pass to the transformer the entities we have detected as being used in this class instead of the whole list of entities. While doing that should make the first optimization a lot less useful, I think it's still good to keep it around.
We used to apply all transformers if at least one of them was not requiring any constant pool check, including the ones for which we had a constant pool check, which was a bad idea: we now filter the transformers properly and we apply the ones that should be applied. Nice bonus: we don't need to prepare some sort of aggregated config before applying the transformers as we don't need one. All in all, it's faster and should be more correct.
95d5954
to
64fd07c
Compare
CI is green in my fork, this is now ready for review. |
Status for workflow
|
Status | Name | Step | Failures | Logs | Raw logs | Build scan |
---|---|---|---|---|---|---|
✔️ | JVM Tests - JDK 17 | Logs | Raw logs | 🚧 | ||
✔️ | JVM Tests - JDK 21 | Logs | Raw logs | 🚧 | ||
❌ | JVM Tests - JDK 17 Windows | Build |
Failures | Logs | Raw logs | 🚧 |
Full information is available in the Build summary check run.
You can consult the Develocity build scans.
Failures
⚙️ JVM Tests - JDK 17 Windows #
- Failing: extensions/vertx-http/deployment
! Skipped: devtools/cli extensions/agroal/deployment extensions/amazon-lambda-http/deployment and 147 more
📦 extensions/vertx-http/deployment
❌ io.quarkus.vertx.http.proxy.TrustedForwarderDnsResolveTest.testTrustedProxyResolved
- History - More details - Source on GitHub
java.io.IOException:
Error while binding on /127.0.0.1:53530
original message : Address already in use: bind
at org.apache.mina.transport.socket.nio.NioSocketAcceptor.open(NioSocketAcceptor.java:238)
at org.apache.mina.transport.socket.nio.NioSocketAcceptor.open(NioSocketAcceptor.java:51)
at org.apache.mina.core.polling.AbstractPollingIoAcceptor.registerHandles(AbstractPollingIoAcceptor.java:585)
at org.apache.mina.core.polling.AbstractPollingIoAcceptor.access$400(AbstractPollingIoAcceptor.java:71)
at org.apache.mina.core.polling.AbstractPollingIoAcceptor$Acceptor.run(AbstractPollingIoAcceptor.java:459)
Flaky tests - Develocity
⚙️ JVM Tests - JDK 17
📦 extensions/smallrye-reactive-messaging/deployment
❌ io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector
- History
Expecting actual: ["-6","-7","-8","-10","-11","-12","-13","-14"] to start with: ["-6", "-7", "-8", "-9"]
-java.lang.AssertionError
java.lang.AssertionError:
Expecting actual:
["-6","-7","-8","-10","-11","-12","-13","-14"]
to start with:
["-6", "-7", "-8", "-9"]
at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:41)
@geoand this one is probably for you. It can wait for you to come back from PTO, it's for 3.28 anyway. |
FWIW, the CI failure is NOT related. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I had a look and this makes a lot of sense to me. But yeah, I would prefer if Georgios could have a look (later).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Note
This is part of my efforts on large monoliths but it should benefit all applications in the end.
In BytecodeTransformerBuildItem, we can define a set of classes which will be used to determine if a class is affected by a transformation by by checking the constant pool.
This is most notably (and I think exclusively) used as an optimization for the Panache transformation of public fields -> accessors.
When you have an application with a lot of entities, all the BytecodeTransformerBuildItem targeting the classes using these entities are carrying the Set of all the entities.
And we used copy the Set over and over for each class affected by the transformation, which was generating a lot of useless allocations.
Moreover, the optimization was not enabled if any other bytecode transformer was not requiring constant pool scanning, which was counter productive.
This patch is constituted of two commits:
To: