From 932336830a7013b95cf8c3e9ae881ba290e0acbc Mon Sep 17 00:00:00 2001 From: Ladislav Thon Date: Mon, 10 Feb 2025 17:21:22 +0100 Subject: [PATCH 1/3] improve javadoc of the EquivalenceKey --- .../java/org/jboss/jandex/EquivalenceKey.java | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/core/src/main/java/org/jboss/jandex/EquivalenceKey.java b/core/src/main/java/org/jboss/jandex/EquivalenceKey.java index 91de1b23..64fb780b 100644 --- a/core/src/main/java/org/jboss/jandex/EquivalenceKey.java +++ b/core/src/main/java/org/jboss/jandex/EquivalenceKey.java @@ -22,7 +22,7 @@ * * In addition, equivalence keys are structured in an inheritance hierarchy that corresponds @@ -34,9 +34,9 @@ *
  • {@code DeclarationEquivalenceKey} * *
    • @@ -59,10 +59,10 @@ */ public abstract class EquivalenceKey { /** - * Returns an equivalence key for given {@link AnnotationTarget annotation target}. + * Returns an equivalence key for given {@linkplain AnnotationTarget annotation target}. * * @param annotationTarget the annotation target, may be {@code null} - * @return equvalence key for given annotation target, only {@code null} if {@code annotationTarget == null} + * @return equivalence key for given annotation target, only {@code null} if {@code annotationTarget == null} */ public static EquivalenceKey of(AnnotationTarget annotationTarget) { if (annotationTarget == null) { @@ -80,10 +80,10 @@ public static EquivalenceKey of(AnnotationTarget annotationTarget) { } /** - * Returns an equivalence key for given {@link Declaration declaration}. + * Returns an equivalence key for given {@linkplain Declaration declaration}. * * @param declaration the declaration, may be {@code null} - * @return equvalence key for given declaration, only {@code null} if {@code declaration == null} + * @return equivalence key for given declaration, only {@code null} if {@code declaration == null} * @since 3.1.0 */ public static DeclarationEquivalenceKey of(Declaration declaration) { @@ -108,10 +108,10 @@ public static DeclarationEquivalenceKey of(Declaration declaration) { } /** - * Returns an equivalence key for given class. + * Returns an equivalence key for given {@linkplain ClassInfo class}. * * @param clazz the class, may be {@code null} - * @return equvalence key for given class, only {@code null} if {@code clazz == null} + * @return equivalence key for given class, only {@code null} if {@code clazz == null} */ public static ClassEquivalenceKey of(ClassInfo clazz) { if (clazz == null) { @@ -121,10 +121,10 @@ public static ClassEquivalenceKey of(ClassInfo clazz) { } /** - * Returns an equivalence key for given method. + * Returns an equivalence key for given {@linkplain MethodInfo method}. * * @param method the method, may be {@code null} - * @return equvalence key for given method, only {@code null} if {@code method == null} + * @return equivalence key for given method, only {@code null} if {@code method == null} */ public static MethodEquivalenceKey of(MethodInfo method) { if (method == null) { @@ -135,10 +135,10 @@ public static MethodEquivalenceKey of(MethodInfo method) { } /** - * Returns an equivalence key for given method parameter. + * Returns an equivalence key for given {@linkplain MethodParameterInfo method parameter}. * * @param parameter the method parameter, may be {@code null} - * @return equvalence key for given method parameter, only {@code null} if {@code parameter == null} + * @return equivalence key for given method parameter, only {@code null} if {@code parameter == null} */ public static MethodParameterEquivalenceKey of(MethodParameterInfo parameter) { if (parameter == null) { @@ -148,10 +148,10 @@ public static MethodParameterEquivalenceKey of(MethodParameterInfo parameter) { } /** - * Returns an equivalence key for given field. + * Returns an equivalence key for given {@linkplain FieldInfo field}. * * @param field the field, may be {@code null} - * @return equvalence key for given field, only {@code null} if {@code field == null} + * @return equivalence key for given field, only {@code null} if {@code field == null} */ public static FieldEquivalenceKey of(FieldInfo field) { if (field == null) { @@ -161,10 +161,10 @@ public static FieldEquivalenceKey of(FieldInfo field) { } /** - * Returns an equivalence key for given record component. + * Returns an equivalence key for given {@linkplain RecordComponentInfo record component}. * * @param recordComponent the record component, may be {@code null} - * @return equvalence key for given record component, only {@code null} if {@code recordComponent == null} + * @return equivalence key for given record component, only {@code null} if {@code recordComponent == null} */ public static RecordComponentEquivalenceKey of(RecordComponentInfo recordComponent) { if (recordComponent == null) { @@ -175,11 +175,11 @@ public static RecordComponentEquivalenceKey of(RecordComponentInfo recordCompone } /** - * Returns an equivalence key for given type annotation target. + * Returns an equivalence key for given {@linkplain TypeTarget type annotation target}. * It is the equivalence key of the annotated type. * * @param typeTarget the type target, may be {@code null} - * @return equvalence key for given type target, only {@code null} if {@code typeTarget == null} + * @return equivalence key for given type target, only {@code null} if {@code typeTarget == null} */ public static TypeEquivalenceKey of(TypeTarget typeTarget) { if (typeTarget == null) { @@ -189,10 +189,10 @@ public static TypeEquivalenceKey of(TypeTarget typeTarget) { } /** - * Returns an equivalence key for given type. + * Returns an equivalence key for given {@linkplain Type type}. * * @param type the type, may be {@code null} - * @return equvalence key for given type, only {@code null} if {@code type == null} + * @return equivalence key for given type, only {@code null} if {@code type == null} */ public static TypeEquivalenceKey of(Type type) { if (type == null) { From afc585f351483163204541069ed4614757a18bb8 Mon Sep 17 00:00:00 2001 From: Ladislav Thon Date: Mon, 10 Feb 2025 14:19:10 +0100 Subject: [PATCH 2/3] improve performance of the annotation overlay There's several improvements in this commit: 1. Transformed annotations are always cached, even if no transformation applied. This makes it much faster in the most common case, while adding no significant memory overhead, because vast majority of objects already existed. 2. The transformation context implementation creates a copy of annotations lazily, on the first need. Again, this makes it much faster in the most common case (which is no annotation transformations applied). 3. Several copies were eliminated because they were just unnecessary overhead. For example, `AnnotationTarget.annotations()` or `declaredAnnotations()` already return unmodifiable collections, so there's no need to rewrap them again. 4. The [potentially transformed] annotations are no longer stored as a `Set` but as a `Collection`. This saves several copies and doesn't affect public API, which has always been defined in terms of `Collection`s. 5. Lambdas in `AnnotationOverlayImpl` were replaced by annonymous classes. The [modest] usage of streams was eliminated completely. --- .../jboss/jandex/AnnotationOverlayImpl.java | 144 +++++++++++++----- .../jandex/MutableAnnotationOverlayImpl.java | 18 ++- 2 files changed, 115 insertions(+), 47 deletions(-) diff --git a/core/src/main/java/org/jboss/jandex/AnnotationOverlayImpl.java b/core/src/main/java/org/jboss/jandex/AnnotationOverlayImpl.java index 5b3c1d43..34c5cf8a 100644 --- a/core/src/main/java/org/jboss/jandex/AnnotationOverlayImpl.java +++ b/core/src/main/java/org/jboss/jandex/AnnotationOverlayImpl.java @@ -11,19 +11,18 @@ import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Function; import java.util.function.Predicate; import org.jboss.jandex.AnnotationTransformation.TransformationContext; class AnnotationOverlayImpl implements AnnotationOverlay { - private static final Set SENTINEL = Collections.unmodifiableSet(new HashSet<>()); - final IndexView index; final boolean compatibleMode; final boolean runtimeAnnotationsOnly; final boolean inheritedAnnotations; final List transformations; - final Map> overlay = new ConcurrentHashMap<>(); + final Map> overlay = new ConcurrentHashMap<>(); AnnotationOverlayImpl(IndexView index, boolean compatibleMode, boolean runtimeAnnotationsOnly, boolean inheritedAnnotations, Collection annotationTransformations) { @@ -216,43 +215,65 @@ public final Collection annotations(Declaration declaration) while (clazz != null && !DotName.OBJECT_NAME.equals(clazz.name())) { for (AnnotationInstance annotation : getAnnotationsFor(clazz)) { ClassInfo annotationClass = index.getClassByName(annotation.name()); - if (annotationClass != null - && annotationClass.hasDeclaredAnnotation(DotName.INHERITED_NAME) - && result.stream().noneMatch(it -> it.name().equals(annotation.name()))) { - result.add(annotation); + if (annotationClass != null && annotationClass.hasDeclaredAnnotation(DotName.INHERITED_NAME)) { + boolean noMatchingAnnotationPresent = true; + for (AnnotationInstance it : result) { + if (it.name().equals(annotation.name())) { + noMatchingAnnotationPresent = false; + break; + } + } + if (noMatchingAnnotationPresent) { + result.add(annotation); + } } } clazz = index.getClassByName(clazz.superName()); } + result = Collections.unmodifiableCollection(result); } - return Collections.unmodifiableCollection(result); + return result; } - Set getAnnotationsFor(Declaration declaration) { + Collection getAnnotationsFor(Declaration declaration) { EquivalenceKey key = EquivalenceKey.of(declaration); - Set annotations = overlay.computeIfAbsent(key, ignored -> { - Set original = getOriginalAnnotations(declaration); - TransformationContextImpl transformationContext = new TransformationContextImpl(declaration, original); - for (AnnotationTransformation transformation : transformations) { - if (transformation.supports(declaration.kind())) { - transformation.apply(transformationContext); + return overlay.computeIfAbsent(key, new Function>() { + @Override + public Collection apply(EquivalenceKey ignored) { + Collection original = getOriginalAnnotations(declaration); + TransformationContextImpl transformationContext = new TransformationContextImpl(declaration, original); + for (AnnotationTransformation transformation : transformations) { + if (transformation.supports(declaration.kind())) { + transformation.apply(transformationContext); + } } + + if (transformationContext.modified()) { + Collection result = transformationContext.annotations; + if (result.isEmpty()) { + return Collections.emptyList(); + } else if (result.size() == 1) { + return Collections.singleton(result.iterator().next()); + } else { + return Collections.unmodifiableCollection(result); + } + } + return original; } - Set result = transformationContext.annotations; - return original.equals(result) ? SENTINEL : Collections.unmodifiableSet(result); }); - - if (annotations == SENTINEL) { - annotations = getOriginalAnnotations(declaration); - } - return annotations; } - final Set getOriginalAnnotations(Declaration declaration) { - Set result = new HashSet<>(); + // always returns an unmodifiable collection + final Collection getOriginalAnnotations(Declaration declaration) { if (compatibleMode && declaration.kind() == AnnotationTarget.Kind.METHOD) { - for (AnnotationInstance annotation : declaration.asMethod().annotations()) { + List annotations = declaration.asMethod().annotations(); + if (annotations.isEmpty()) { + return Collections.emptyList(); + } + + List result = new ArrayList<>(annotations.size()); + for (AnnotationInstance annotation : annotations) { if (annotation.target() != null && (annotation.target().kind() == AnnotationTarget.Kind.METHOD || annotation.target().kind() == AnnotationTarget.Kind.METHOD_PARAMETER) @@ -260,23 +281,43 @@ final Set getOriginalAnnotations(Declaration declaration) { result.add(annotation); } } - } else { - for (AnnotationInstance annotation : declaration.declaredAnnotations()) { - if (!runtimeAnnotationsOnly || annotation.runtimeVisible()) { - result.add(annotation); - } + return Collections.unmodifiableList(result); + } + + Collection annotations = declaration.declaredAnnotations(); + + if (!runtimeAnnotationsOnly) { + return annotations; + } + + List result = new ArrayList<>(annotations.size()); + for (AnnotationInstance annotation : annotations) { + if (annotation.runtimeVisible()) { + result.add(annotation); } } - return result; + return Collections.unmodifiableList(result); } private static final class TransformationContextImpl implements TransformationContext { private final Declaration declaration; - private final Set annotations; + private final Collection originalAnnotations; + private Set annotations; TransformationContextImpl(Declaration declaration, Collection annotations) { this.declaration = declaration; - this.annotations = new HashSet<>(annotations); + this.originalAnnotations = annotations; + this.annotations = null; + } + + boolean modified() { + return annotations != null; + } + + private void initializeIfNecessary() { + if (annotations == null) { + annotations = new HashSet<>(originalAnnotations); + } } @Override @@ -286,6 +327,7 @@ public Declaration declaration() { @Override public Collection annotations() { + initializeIfNecessary(); return annotations; } @@ -298,7 +340,7 @@ public boolean hasAnnotation(Class annotationClass) { @Override public boolean hasAnnotation(DotName annotationName) { Objects.requireNonNull(annotationName); - for (AnnotationInstance annotation : annotations) { + for (AnnotationInstance annotation : modified() ? annotations : originalAnnotations) { if (annotation.name().equals(annotationName)) { return true; } @@ -309,7 +351,7 @@ public boolean hasAnnotation(DotName annotationName) { @Override public boolean hasAnnotation(Predicate predicate) { Objects.requireNonNull(predicate); - for (AnnotationInstance annotation : annotations) { + for (AnnotationInstance annotation : modified() ? annotations : originalAnnotations) { if (predicate.test(annotation)) { return true; } @@ -319,20 +361,27 @@ public boolean hasAnnotation(Predicate predicate) { @Override public void add(Class annotationClass) { + initializeIfNecessary(); + Objects.requireNonNull(annotationClass); annotations.add(AnnotationInstance.builder(annotationClass).buildWithTarget(declaration)); } @Override public void add(AnnotationInstance annotation) { + initializeIfNecessary(); + + Objects.requireNonNull(annotation); if (annotation.target() == null) { annotation = AnnotationInstance.create(annotation, declaration); } - annotations.add(Objects.requireNonNull(annotation)); + annotations.add(annotation); } @Override public void addAll(AnnotationInstance... annotations) { + initializeIfNecessary(); + Objects.requireNonNull(annotations); for (int i = 0; i < annotations.length; i++) { if (annotations[i].target() == null) { @@ -344,8 +393,17 @@ public void addAll(AnnotationInstance... annotations) { @Override public void addAll(Collection annotations) { + initializeIfNecessary(); + Objects.requireNonNull(annotations); - if (annotations.stream().anyMatch(it -> it.target() == null)) { + boolean hasNullTarget = false; + for (AnnotationInstance annotation : annotations) { + if (annotation.target() == null) { + hasNullTarget = true; + break; + } + } + if (hasNullTarget) { List fixed = new ArrayList<>(); for (AnnotationInstance annotation : annotations) { if (annotation.target() == null) { @@ -361,12 +419,20 @@ public void addAll(Collection annotations) { @Override public void remove(Predicate predicate) { - annotations.removeIf(Objects.requireNonNull(predicate)); + initializeIfNecessary(); + + Objects.requireNonNull(predicate); + annotations.removeIf(predicate); } @Override public void removeAll() { - annotations.clear(); + // skipping `initializeIfNecessary()` here, because that would do useless work + if (modified()) { + annotations.clear(); + } else { + annotations = new HashSet<>(); + } } } } diff --git a/core/src/main/java/org/jboss/jandex/MutableAnnotationOverlayImpl.java b/core/src/main/java/org/jboss/jandex/MutableAnnotationOverlayImpl.java index e53b9f44..f3f30bd1 100644 --- a/core/src/main/java/org/jboss/jandex/MutableAnnotationOverlayImpl.java +++ b/core/src/main/java/org/jboss/jandex/MutableAnnotationOverlayImpl.java @@ -1,8 +1,10 @@ package org.jboss.jandex; +import java.util.Collection; import java.util.Collections; +import java.util.HashSet; import java.util.List; -import java.util.Set; +import java.util.function.Function; import java.util.function.Predicate; final class MutableAnnotationOverlayImpl extends AnnotationOverlayImpl implements MutableAnnotationOverlay { @@ -14,14 +16,14 @@ final class MutableAnnotationOverlayImpl extends AnnotationOverlayImpl implement } @Override - Set getAnnotationsFor(Declaration declaration) { + Collection getAnnotationsFor(Declaration declaration) { EquivalenceKey key = EquivalenceKey.of(declaration); - Set annotations = overlay.get(key); - if (annotations == null) { - annotations = getOriginalAnnotations(declaration); - overlay.put(key, annotations); - } - return annotations; + return overlay.computeIfAbsent(key, new Function>() { + @Override + public Collection apply(EquivalenceKey ignored) { + return new HashSet<>(getOriginalAnnotations(declaration)); + } + }); } @Override From f8ffd4c5f379c6169d8dde088180dbb9a31b3b8b Mon Sep 17 00:00:00 2001 From: Ladislav Thon Date: Tue, 11 Feb 2025 11:21:30 +0100 Subject: [PATCH 3/3] add benchmark for the annotation overlay The benchmark is rather simplistic and cannot be used in isolation. A real-world benchmark needs to be used as well. --- benchmarks/run-benchmarks.sh | 2 +- .../jboss/jandex/chart/ChartGenerator.java | 2 +- .../jandex/AnnotationOverlayBenchmark.java | 193 ++++++++++++++++++ 3 files changed, 195 insertions(+), 2 deletions(-) create mode 100644 benchmarks/src/test/java/org/jboss/jandex/AnnotationOverlayBenchmark.java diff --git a/benchmarks/run-benchmarks.sh b/benchmarks/run-benchmarks.sh index c3408642..bb21d31e 100755 --- a/benchmarks/run-benchmarks.sh +++ b/benchmarks/run-benchmarks.sh @@ -4,7 +4,7 @@ VERSIONS="HEAD" BENCHMARKS=".*" help() { - echo "Usage: run-benchmarks.sh [--versions ] [--benchmars ] [--help]" + echo "Usage: run-benchmarks.sh [--versions ] [--benchmarks ] [--help]" echo echo " --versions : comma-separated list of Jandex versions (Git commits) to benchmark" echo " defaults to 'HEAD'" diff --git a/benchmarks/src/main/java/org/jboss/jandex/chart/ChartGenerator.java b/benchmarks/src/main/java/org/jboss/jandex/chart/ChartGenerator.java index 3aa59477..7eaf3500 100644 --- a/benchmarks/src/main/java/org/jboss/jandex/chart/ChartGenerator.java +++ b/benchmarks/src/main/java/org/jboss/jandex/chart/ChartGenerator.java @@ -64,7 +64,7 @@ public static void main(String[] args) throws IOException { .build(); chart.getStyler() .setXAxisTicksVisible(true) - .setXAxisLabelRotation(90); + .setXAxisLabelRotation(45); for (BenchmarksForVersion benchmarksForVersion : allVersions) { List names = new ArrayList<>(); diff --git a/benchmarks/src/test/java/org/jboss/jandex/AnnotationOverlayBenchmark.java b/benchmarks/src/test/java/org/jboss/jandex/AnnotationOverlayBenchmark.java new file mode 100644 index 00000000..c0413150 --- /dev/null +++ b/benchmarks/src/test/java/org/jboss/jandex/AnnotationOverlayBenchmark.java @@ -0,0 +1,193 @@ +package org.jboss.jandex; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; + +@BenchmarkMode(Mode.Throughput) +@Fork(5) +@Warmup(iterations = 5, time = 1, batchSize = 8192) +@Measurement(iterations = 5, time = 1, batchSize = 8192) +@State(Scope.Benchmark) +public class AnnotationOverlayBenchmark { + private Index index; + private AnnotationOverlay overlay; + + @Setup + public void setup() throws IOException { + index = Index.of(MyAnn1.class, MyAnn2.class, MyClass1.class, MyClass2.class, MyClass3.class, + MyClass4.class, MyClass5.class, MyClass6.class, MyClass7.class, MyClass8.class, MyClass9.class); + + AnnotationTransformation transformation = new AnnotationTransformation() { + @Override + public boolean supports(AnnotationTarget.Kind kind) { + return kind == AnnotationTarget.Kind.CLASS; + } + + @Override + public void apply(TransformationContext context) { + String name = context.declaration().asClass().name().toString(); + if (name.endsWith("2") || name.endsWith("4")) { + context.removeAll(); + } else if (name.endsWith("6") || name.endsWith("8")) { + context.add(MyAnn2.class); + } + } + }; + overlay = AnnotationOverlay.builder(index, Collections.singleton(transformation)).build(); + } + + @Benchmark + public List noTransformation() { + List result = new ArrayList<>(5); + + ClassInfo clazz = index.getClassByName(MyClass1.class); + result.addAll(overlay.annotations(clazz)); + + clazz = index.getClassByName(MyClass3.class); + result.addAll(overlay.annotations(clazz)); + + clazz = index.getClassByName(MyClass5.class); + result.addAll(overlay.annotations(clazz)); + + clazz = index.getClassByName(MyClass7.class); + result.addAll(overlay.annotations(clazz)); + + clazz = index.getClassByName(MyClass9.class); + result.addAll(overlay.annotations(clazz)); + + return result; + } + + @Benchmark + public List removedAnnotations() { + List result = new ArrayList<>(0); + + ClassInfo clazz = index.getClassByName(MyClass2.class); + result.addAll(overlay.annotations(clazz)); + + clazz = index.getClassByName(MyClass4.class); + result.addAll(overlay.annotations(clazz)); + + return result; + } + + @Benchmark + public List addedAnnotations() { + List result = new ArrayList<>(4); + + ClassInfo clazz = index.getClassByName(MyClass6.class); + result.addAll(overlay.annotations(clazz)); + + clazz = index.getClassByName(MyClass8.class); + result.addAll(overlay.annotations(clazz)); + + return result; + } + + @Benchmark + public List addedAndRemovedAnnotations() { + List result = new ArrayList<>(4); + + ClassInfo clazz = index.getClassByName(MyClass2.class); + result.addAll(overlay.annotations(clazz)); + + clazz = index.getClassByName(MyClass4.class); + result.addAll(overlay.annotations(clazz)); + + clazz = index.getClassByName(MyClass6.class); + result.addAll(overlay.annotations(clazz)); + + clazz = index.getClassByName(MyClass8.class); + result.addAll(overlay.annotations(clazz)); + + return result; + } + + @Benchmark + public List allClasses() { + List result = new ArrayList<>(9); + + ClassInfo clazz = index.getClassByName(MyClass1.class); + result.addAll(overlay.annotations(clazz)); + + clazz = index.getClassByName(MyClass2.class); + result.addAll(overlay.annotations(clazz)); + + clazz = index.getClassByName(MyClass3.class); + result.addAll(overlay.annotations(clazz)); + + clazz = index.getClassByName(MyClass4.class); + result.addAll(overlay.annotations(clazz)); + + clazz = index.getClassByName(MyClass5.class); + result.addAll(overlay.annotations(clazz)); + + clazz = index.getClassByName(MyClass6.class); + result.addAll(overlay.annotations(clazz)); + + clazz = index.getClassByName(MyClass7.class); + result.addAll(overlay.annotations(clazz)); + + clazz = index.getClassByName(MyClass8.class); + result.addAll(overlay.annotations(clazz)); + + clazz = index.getClassByName(MyClass9.class); + result.addAll(overlay.annotations(clazz)); + + return result; + } + + public @interface MyAnn1 { + } + + public @interface MyAnn2 { + } + + @MyAnn1 + public static class MyClass1 { + } + + @MyAnn1 + public static class MyClass2 { + } + + @MyAnn1 + public static class MyClass3 { + } + + @MyAnn1 + public static class MyClass4 { + } + + @MyAnn1 + public static class MyClass5 { + } + + @MyAnn1 + public static class MyClass6 { + } + + @MyAnn1 + public static class MyClass7 { + } + + @MyAnn1 + public static class MyClass8 { + } + + @MyAnn1 + public static class MyClass9 { + } +}