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

Reviewed some Quarkus core classes #40630

Merged
merged 3 commits into from
May 15, 2024
Merged

Reviewed some Quarkus core classes #40630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
067ae84
Simplify RecordingAnnotationsUtil
Sanne May 13, 2024
c82ed89
Potential problem in recognizing boolean getters
Sanne May 13, 2024
ad96389
Avoid o^2 lookup for Fields, avoid NoSuchFieldException(s)
Sanne May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions core/deployment/src/main/java/io/quarkus/deployment/recording/BytecodeRecorderImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1200,20 +1200,17 @@ public void prepare(MethodContext context) {

Set<String> handledProperties = new HashSet<>();
Property[] desc = PropertyUtils.getPropertyDescriptors(param);
FieldsHelper fieldsHelper = new FieldsHelper(param.getClass());
for (Property i : desc) {
if (!i.getDeclaringClass().getPackageName().startsWith("java.")) {
// check if the getter is ignored
if ((i.getReadMethod() != null) && RecordingAnnotationsUtil.isIgnored(i.getReadMethod())) {
continue;
}
// check if the matching field is ignored
try {
Field field = param.getClass().getDeclaredField(i.getName());
if (ignoreField(field)) {
continue;
}
} catch (NoSuchFieldException ignored) {

Field field = fieldsHelper.getDeclaredField(i.getName());
if (field != null && ignoreField(field)) {
continue;
}
}
Integer ctorParamIndex = constructorParamNameMap.remove(i.name);
Expand Down
23 changes: 23 additions & 0 deletions core/deployment/src/main/java/io/quarkus/deployment/recording/FieldsHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package io.quarkus.deployment.recording;

import java.lang.reflect.Field;
import java.util.HashMap;
import java.util.Map;

final class FieldsHelper {

private final Map<String, Field> fields;

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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 :)

for (Field field : declaredFields) {
this.fields.put(field.getName(), field);
}
}

//Returns the matching Field, or null if not existing
public Field getDeclaredField(final String name) {
return fields.get(name);
}
}
2 changes: 1 addition & 1 deletion core/deployment/src/main/java/io/quarkus/deployment/recording/PropertyUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

&& (i.getReturnType() == boolean.class || i.getReturnType() == Boolean.class)) {
String name = Character.toLowerCase(i.getName().charAt(2)) + i.getName().substring(3);
isGetters.put(name, i);
Expand Down
37 changes: 19 additions & 18 deletions core/deployment/src/main/java/io/quarkus/deployment/recording/RecordingAnnotationsUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import java.lang.reflect.AccessibleObject;
import java.lang.reflect.Constructor;
import java.util.HashSet;
import java.util.List;
import java.util.ServiceLoader;
import java.util.Set;

Expand All @@ -13,8 +12,8 @@

final class RecordingAnnotationsUtil {

static final List<Class<? extends Annotation>> IGNORED_PROPERTY_ANNOTATIONS;
static final List<Class<? extends Annotation>> RECORDABLE_CONSTRUCTOR_ANNOTATIONS;
private static final Class<? extends Annotation>[] IGNORED_PROPERTY_ANNOTATIONS;
private static final Class<? extends Annotation>[] RECORDABLE_CONSTRUCTOR_ANNOTATIONS;

static {
Set<Class<? extends Annotation>> ignoredPropertyAnnotations = new HashSet<>();
Expand All @@ -33,30 +32,32 @@ final class RecordingAnnotationsUtil {
}
}

IGNORED_PROPERTY_ANNOTATIONS = List.copyOf(ignoredPropertyAnnotations);
RECORDABLE_CONSTRUCTOR_ANNOTATIONS = List.copyOf(recordableConstructorAnnotations);
IGNORED_PROPERTY_ANNOTATIONS = ignoredPropertyAnnotations.toArray(new Class[0]);
RECORDABLE_CONSTRUCTOR_ANNOTATIONS = recordableConstructorAnnotations.toArray(new Class[0]);
Comment on lines +35 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

compiler knows this pattern well

kk

}

private RecordingAnnotationsUtil() {
}

static boolean isIgnored(AccessibleObject object) {
for (int i = 0; i < IGNORED_PROPERTY_ANNOTATIONS.size(); i++) {
Class<? extends Annotation> annotation = IGNORED_PROPERTY_ANNOTATIONS.get(i);
if (object.isAnnotationPresent(annotation)) {
return true;
}
}
return false;
static boolean isIgnored(final AccessibleObject object) {
return annotationsMatch(object.getDeclaredAnnotations(), IGNORED_PROPERTY_ANNOTATIONS);
}

static boolean isRecordableConstructor(Constructor<?> ctor) {
for (int i = 0; i < RECORDABLE_CONSTRUCTOR_ANNOTATIONS.size(); i++) {
Class<? extends Annotation> annotation = RECORDABLE_CONSTRUCTOR_ANNOTATIONS.get(i);
if (ctor.isAnnotationPresent(annotation)) {
return true;
static boolean isRecordableConstructor(final Constructor<?> ctor) {
return annotationsMatch(ctor.getDeclaredAnnotations(), RECORDABLE_CONSTRUCTOR_ANNOTATIONS);
}

private static boolean annotationsMatch(
final Annotation[] declaredAnnotations,
final Class<? extends Annotation>[] typesToCheck) {
for (Class<? extends Annotation> annotation : typesToCheck) {
for (Annotation declaredAnnotation : declaredAnnotations) {
if (declaredAnnotation.annotationType().equals(annotation)) {
return true;
}
}
}
return false;
}

}
Loading