ArC - remove unused interceptors #18970
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
mkouba
commented
Jul 23, 2021
mkouba
commented
- and beans injected only by these interceptors
- and beans that are only injected in removed beans
|
Things to add:
|
Awesome! |
mkouba
force-pushed
the
arc-remove-unused-interceptors
branch
from
42a7462
to
431b064
Compare
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 431b064
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 extensions/mailer/deployment✖ ✖ 📦 extensions/smallrye-fault-tolerance/deployment✖ ⚙️ JVM Tests - JDK 11 Windows #📦 extensions/mailer/deployment✖ ✖ 📦 extensions/smallrye-fault-tolerance/deployment✖ ⚙️ JVM Tests - JDK 16 #📦 extensions/mailer/deployment✖ ✖ 📦 extensions/smallrye-fault-tolerance/deployment✖ ⚙️ Native Tests - Cache #📦 integration-tests/cache✖ ⚙️ Native Tests - Data4 #📦 integration-tests/mongodb-panache-kotlin✖ ✖ ✖ ✖ ✖ ✖ |
mkouba
force-pushed
the
arc-remove-unused-interceptors
branch
from
431b064
to
16fcd22
Compare
Ladicek
reviewed
Jul 26, 2021
independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/BeanDeployment.java
Outdated
Show resolved
Hide resolved
|
Looks good to me, but my knowledge of ArC is very superficial. Regarding the algorithm: I guess it's a best effort, to be really sure to remove it all it should run in multiple phases until it neither any bean nor any interceptor is removed, no? Also wondering: would you want to merge this now, or rather wait for 2.3+ ? |
|
Looks fine on the first sight, though I'd have to check it out locally and walk around the code for a few hours to be confident :-) @Sanne I think this method does a fixed point iteration, which seems to be what you're asking for? |
|
Ah I probably got it wrong. Indeed currently removing unused interceptors/decorators is only a single pass, which is followed by removing unused beans (which is iterated). I don't really know if it's possible for unused beans to declare interceptors that would become unused after the unused beans are removed, but I guess it might. |
|
Right, I think (if we wanted to really cover it all) something similar would need to be done while taking into account both beans and interceptors. But I'm not sure if it's worth it - both in terms of complexity/maintenance and in terms of complexity/execution times. Was just bringing it up as I'm curious if it was a conscious choice. |
|
OK, even better. So we remove unused beans at the beginning, then remove unused interceptors/decorators (that's single pass), then we remove unused beans again. I think it should be something like this instead (pseudocode): boolean shouldContinue = true;
while (shouldContinue) {
boolean someBeansRemoved = removeUnusedBeans(...);
boolean someInterceptorsRemoved = removeUnusedInterceptors(...);
boolean someDecoratorsRemoved = removeUnusedDecorators(...);
shouldContinue = someBeansRemoved || someInterceptorsRemoved || someDecoratorsRemoved;
} |
Sanne
approved these changes
Jul 26, 2021
There was a problem hiding this comment.
I'll approve so that @mkouba can merge if he wants to 👍
I'd be keen to measure it but my pet projects don't really use interceptors so I won't do this yet. I might do another pass to measure additional memory costs in ArcContainerImpl, but I do those in batches later.
Yes, I think that in theory it's possible. For example if you have a bean
That's a good question. It should probably target 2.3 because we may expect problems with programmatic lookup in extensions that do rely on less strict implementation (although the rules stay the same ;-).
Yes, it's a best effort. But we should probably try to cover maximum use cases... |
|
Moving the fixed point iteration up one level shouldn't be hard, from my superficial understanding of the code. If @mkouba doesn't get to it, I may try later :-) |
|
I'm on it but I'll ping you for review ;-) |
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 16fcd22
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 extensions/resteasy-classic/rest-client/deployment✖ ✖ ⚙️ JVM Tests - JDK 11 Windows #📦 extensions/resteasy-classic/rest-client/deployment✖ ✖ ⚙️ JVM Tests - JDK 16 #📦 extensions/resteasy-classic/rest-client/deployment✖ ✖ ⚙️ Native Tests - Cache #📦 integration-tests/cache✖ ⚙️ Native Tests - Data4 #📦 integration-tests/mongodb-panache-kotlin✖ ✖ ✖ ✖ ✖ ✖ |
mkouba
force-pushed
the
arc-remove-unused-interceptors
branch
from
16fcd22
to
d210858
Compare
Ladicek
approved these changes
Jul 26, 2021
Don't be afraid I will not blame you if we find a critical bug in the relevant part of the code... or will I? ;-) |
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 2de5b0c
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 integration-tests/smallrye-config✖ ✖ ✖ ✖ 📦 integration-tests/smallrye-opentracing✖ ⚙️ JVM Tests - JDK 11 Windows #📦 integration-tests/smallrye-config✖ ✖ ✖ ✖ ✖ 📦 integration-tests/smallrye-opentracing✖ ⚙️ JVM Tests - JDK 16 #📦 extensions/resteasy-reactive/rest-client-reactive/deployment✖ 📦 integration-tests/smallrye-config✖ ✖ ✖ ✖ 📦 integration-tests/smallrye-opentracing✖ |
|
All CI jobs are green except for I don't think it's related so I'm going to rebase and squash all commits. |
mkouba
force-pushed
the
arc-remove-unused-interceptors
branch
from
9e34937
to
13fc068
Compare
|
It is not related. It's some race condition that I yet to track down |
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 13fc068
Full information is available in the Build summary check run. Test Failures⚙️ MicroProfile TCKs Tests #📦 tcks/microprofile-fault-tolerance✖ |
|
@Ladicek Just out of curiosity - have you ever seen this ^ |
|
@mkouba Yea that's flaky (smallrye/smallrye-fault-tolerance#233), but it seems to only occur in Quarkus :-) I've never seen it in SRye Fault Tolerance CI. |
|
Ok, so this PR is ready but should probably wait until the main branch corresponds to 2.3. @gsmet WDYT? |
mkouba
force-pushed
the
arc-remove-unused-interceptors
branch
from
13fc068
to
4d41128
Compare
OTOH according to the release planning this PR would have to wait at least few weeks, which is acceptable but may cause headaches while resolving conflicts ;-). |
gsmet
previously approved these changes
Aug 5, 2021
gsmet
previously requested changes
Aug 5, 2021
mkouba
force-pushed
the
arc-remove-unused-interceptors
branch
from
4d41128
to
d9b73b1
Compare
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building d9b73b1
|
- and beans injected only by these interceptors and decorators - and beans that are only injected in removed beans - log the full stack trace if DEBUG level is enabled - fix JTA and SR CP integration - - the tx manager is obtained via CDI.current().select(TransactionManager.class) in the JtaContextProvider - fix SmallRye Metrics - MetricRegistries must be unremovable - fix cache and rest-client integration - fix SmallRye OpenTracing - make any bean that has io.opentracing.Tracer in bean types unremovable - update the docs
mkouba
force-pushed
the
arc-remove-unused-interceptors
branch
from
d9b73b1
to
c547606
Compare
|
I've also prepared a short blogpost about this optimization: https://github.com/mkouba/quarkusio.github.io-1/tree/unused-beans-next |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.