Reviewed some Quarkus core classes #40630

Sanne · 2024-05-14T13:31:35Z

As I was profiling the bootstrap process, I spotted some small inefficiencies and a couple of potential bugs.
Best reviewed by individual commits, as each change is independent - wrapped together as they are all very small changes.

geoand

Very nice!

geoand · 2024-05-14T13:33:55Z

core/deployment/src/main/java/io/quarkus/deployment/recording/RecordingAnnotationsUtil.java

+        IGNORED_PROPERTY_ANNOTATIONS = ignoredPropertyAnnotations.toArray(new Class[0]);
+        RECORDABLE_CONSTRUCTOR_ANNOTATIONS = recordableConstructorAnnotations.toArray(new Class[0]);


As we're making things more efficient, we can make new Class[0] a constant, no?

I'd say it's unnecessary - compiler knows this pattern well. Also, any static constant takes memory forever :P

compiler knows this pattern well

kk

geoand · 2024-05-14T13:34:24Z

core/deployment/src/main/java/io/quarkus/deployment/recording/PropertyUtils.java

@@ -43,7 +43,7 @@ public Property[] apply(Class<?> type) {
                    if (existingGetter == null || existingGetter.getReturnType().isAssignableFrom(i.getReturnType())) {
                        getters.put(name, i);
                    }
-                } else if (i.getName().startsWith("is") && i.getName().length() > 3 && i.getParameterCount() == 0
+                } else if (i.getName().startsWith("is") && i.getName().length() > 2 && i.getParameterCount() == 0


geoand · 2024-05-14T13:35:05Z

core/deployment/src/main/java/io/quarkus/deployment/recording/PropertyUtils.java

-    static final ConcurrentMap<Class<?>, Property[]> CACHE = new ConcurrentHashMap<>();
-
-    private static final Function<Class<?>, Property[]> FUNCTION = new Function<Class<?>, Property[]>() {
+    private static final ClassValue<Property[]> CACHE = new ClassValue<Property[]>() {


N.B. I'm not sure the original code would leak, but this seems much safer to me. I'm referring to classloader leaks in case there's a recorder triggered as this is only capturing properties of recorders so I don't think it would happen.

gsmet · 2024-05-14T16:07:03Z

core/deployment/src/main/java/io/quarkus/deployment/recording/FieldsHelper.java

+
+    public FieldsHelper(final Class<?> aClass) {
+        final Field[] declaredFields = aClass.getDeclaredFields();
+        this.fields = new HashMap<>(declaredFields.length);


Not sure it's a good idea to do that, your Map will be resized always given the default loadFactor of 0.75.

In HV, we had a util who correctly created a Map of the right size for all the maps initialized at bootstrap: https://github.com/hibernate/hibernate-validator/blob/729cd018599d046aa5db1dbe35700521dd596c4e/engine/src/main/java/org/hibernate/validator/internal/util/CollectionHelper.java#L142-L147 .

And sure, it's a micro optimization but in your case, you might create more harm than none by always sizing the map a bit too small.

what are you suggesting, would you prefer I simply use new HashMap<>(); ?

FWIW I know about the optimisation in Validator and we had a similar thing in ORM, but it's not taking into account the likelyhood of hash collisions.. that's why there is a load factor in the hashmap implementation: it's hard to estimate perfectly. One could argue that the default loadfactor is a bit low.. My rule of thumb is currently that as long as we resize just a couple times it's fine, so I like passing the estimated size (when we have it) so to have it right at least within order of magnitude. I guess since in this case we expect the lifespan of the map to be short one should rather overestimate: I'll shift it right once so to double its size?

BTW just checked, on recent JVMs the HashMap implementation is different and it looks like that this is no longer a problem.

Might be time to simplify Validator as well :)

Sanne · 2024-05-14T20:49:07Z

Something seems way off .. I'll need to separate these.

quarkus-bot · 2024-05-15T02:03:22Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit ad96389.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17 Windows

📦 integration-tests/grpc-hibernate

✖ com.example.grpc.hibernate.BlockingRawTest.shouldAdd - History

Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at com.example.grpc.hibernate.BlockingRawTestBase.shouldAdd(BlockingRawTestBase.java:59)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)

Sanne · 2024-05-15T07:04:34Z

I had it run again after removing the one fix which was introducing the ClassValue, and it's passing now. So I'll merge these and scrutinize my understanding of ClassValue ..

geoand · 2024-05-15T07:10:40Z

Very weird... I'd like to know why ClassValue was problems, if anything I expected to be better :)

Sanne · 2024-05-15T08:30:54Z

Very weird... I'd like to know why ClassValue was problems, if anything I expected to be better :)

Same. I'll send a PR with that change individually - should have done so from the beginning but didn't want to flood CI (and reviewers) with all the noise from small changes.

#40651

Sanne requested a review from geoand May 14, 2024 13:31

quarkus-bot bot added the area/core label May 14, 2024

geoand approved these changes May 14, 2024

View reviewed changes

Sanne force-pushed the CoreSimplifications branch from f682f4f to 92e5a67 Compare May 14, 2024 16:03

gsmet reviewed May 14, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

Sanne added 3 commits May 14, 2024 22:20

Simplify RecordingAnnotationsUtil

067ae84

Potential problem in recognizing boolean getters

c82ed89

Avoid o^2 lookup for Fields, avoid NoSuchFieldException(s)

ad96389

Sanne force-pushed the CoreSimplifications branch from 92e5a67 to ad96389 Compare May 14, 2024 21:21

quarkus-bot bot added the triage/flaky-test label May 15, 2024

Sanne merged commit 821e889 into quarkusio:main May 15, 2024
51 checks passed

quarkus-bot bot added this to the 3.11 - main milestone May 15, 2024

Sanne deleted the CoreSimplifications branch May 15, 2024 08:20

Sanne mentioned this pull request May 15, 2024

Exploring potential issue with ClassValue #40651

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reviewed some Quarkus core classes #40630

Reviewed some Quarkus core classes #40630

Sanne commented May 14, 2024

geoand left a comment

geoand May 14, 2024

Sanne May 14, 2024

geoand May 14, 2024

geoand May 14, 2024

geoand May 14, 2024

Sanne May 14, 2024

geoand May 14, 2024

gsmet May 14, 2024

Sanne May 14, 2024

Sanne May 14, 2024

This comment has been minimized.

Sanne commented May 14, 2024

quarkus-bot bot commented May 15, 2024 •

edited by github-actions bot

Loading

Sanne commented May 15, 2024

geoand commented May 15, 2024

Sanne commented May 15, 2024

		IGNORED_PROPERTY_ANNOTATIONS = ignoredPropertyAnnotations.toArray(new Class[0]);
		RECORDABLE_CONSTRUCTOR_ANNOTATIONS = recordableConstructorAnnotations.toArray(new Class[0]);

Reviewed some Quarkus core classes #40630

Reviewed some Quarkus core classes #40630

Conversation

Sanne commented May 14, 2024

geoand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Sanne commented May 14, 2024

quarkus-bot bot commented May 15, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17 Windows

📦 integration-tests/grpc-hibernate

Sanne commented May 15, 2024

geoand commented May 15, 2024

Sanne commented May 15, 2024

quarkus-bot bot commented May 15, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`