Small assorted optimizations for ArC #49555
Conversation
Instead of transforming all the submitted DotNames to toString(), we only submit the ones we test against.
And also avoiding creating an additional list with the Jandex call.
It's going to be a bit larger than required in some cases, but it will avoid resizing the list, which should be a win in most cases.
It triggers unnecessary allocations and it's visible when there are a lot of beans.
Marko told me that it was actually faster to iterate and from what I can see it's true. I won't change it everywhere but it seems like a good idea to make the change in the Core parts. Note that some of the changes I made might not be critical but it wasn't worth the hassle to check everywhere if it actually made a noticeable difference given the size of the change.
quarkus-bot
bot
added
area/arc
| if (arcConfig.shouldEnableBeanRemoval()) { | ||
| Set<String> unremovableClassNames = unremovableBeans.stream() | ||
| Set<DotName> unremovableClassNames = unremovableBeans.stream() | ||
| .map(UnremovableBeanBuildItem::getClassNames) | ||
| .flatMap(Collection::stream) | ||
| .collect(toSet()); | ||
|
|
||
| for (ConfigClassBuildItem configClass : configMappingTypes.values()) { | ||
| if (configClass.getConfigClass().isAnnotationPresent(Unremovable.class) | ||
| || unremovableClassNames.contains(configClass.getName().toString())) { | ||
| || unremovableClassNames.contains(configClass.getName())) { | ||
| toRegister.add(new ConfigMappingBuildItem(configClass.getConfigClass(), configClass.getPrefix())); | ||
| } | ||
| } |
There was a problem hiding this comment.
@mkouba I would appreciate if you could have a look at this as it looks extremely odd. I don't think it should be done this way: it's not actually applying the predicates but relying on the class names being set, which won't work if you have a custom predicate.
I can understand it given you don't have the BeanInfo but I wonder if there could be a better way to achieve the same goal by using ArC metadata?
It's a separate work though, I just kept the logic as is for now.
It was added by @radcortez here: 564d112 .
There was a problem hiding this comment.
This is not correct indeed.
Given the fact that this logic is executed after the validation phase (unused beans are already removed if needed) I think that the correct solution would be to attempt to find a bean for a given config mapping class and if it's present then register the ConfigMappingBuildItem.
@radcortez Something like this should IMO work:
for (ConfigClassBuildItem configClass : configMappingTypes.values()) {
if (!validationPhase.getContext().beans().withBeanClass(configClass.getConfigClass()).isEmpty()) {
toRegister.add(new ConfigMappingBuildItem(configClass.getConfigClass(), configClass.getPrefix()));
}
}There was a problem hiding this comment.
Also, when we clean this up, let's get rid of the classNames field in UnremovableBuildItem. And I think we could also drop the unnecessary public constructors that were added in this PR.
There was a problem hiding this comment.
And I think we could also drop the unnecessary public constructors
We should deprecate them first...
independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/UnusedBeans.java
Show resolved
Hide resolved
|
@Ladicek FWIW, I added one example of the improvements made here in the description. |
Status for workflow
|
|
🎊 PR Preview 9beaf39 has been successfully built and deployed to https://quarkus-pr-main-49555-preview.surge.sh/version/main/guides/
|
Status for workflow
|
Better reviewed commit per commit as I made them semantic.
These optimizations are NOT CPU related: I was having a look at memory allocation profiles.
An example of what we're trying to solve here.
Before:
After: