FISH-5938 5530 improve cdi scan #5531

sgflt · 2021-12-09T20:03:20Z

Testing Performed

Deployment of testcase finished in 5 seconds.

- removed redundant contains on set - list replaced by set - exclusions extracted because there is no need to discover the same annotation dozentimes and define it as excluded over and over again

- if the annotation is cdi enabling, then it does not matter whether it is excluded or not

- there we want to list all classes with annotation - if we exclude some custom annotation then only the first class with that annotation was included

svendiedrichsen · 2021-12-12T19:18:07Z

appserver/web/gf-weld-connector/src/main/java/org/glassfish/weld/connector/WeldUtils.java

-                            }
+                    for (final AnnotationModel am : type.getAnnotations()) {
+                        final AnnotationType at = am.getType();
+                        if (isCDIEnablingAnnotation(at, exclusions) && type.wasDefinedIn(paths)) {


Could this be improved by checking type.wasDefinedIn(paths) on the outer if/for? Something like !(type instanceof AnnotationType) && type.wasDefinedIn(paths). This would avoid annotation analysis for some types.

svendiedrichsen · 2021-12-12T23:47:51Z

@sgflt Could the prefilled annotation lists be converted to sets? This would speed up contains calls when checking annotation names.

svendiedrichsen · 2021-12-13T17:08:19Z

@sgflt Could the prefilled annotation lists be converted to sets? This would speed up contains calls when checking annotation names.

Sorry for being vague on this and of course it wouldn't be as dramatic an improvement as your changes already are. I'm talking about these:

protected static final List<String> cdiScopeAnnotations
protected static final List<String> cdiEnablingAnnotations
protected static final List<String> excludedAnnotationTypes

If those were Sets it would improve on speed a little bit when checking for a class name being contained. And because they are queried at such a high frequency it could make some noticable difference.

- as they are used for heavy lookups

- this small subset of well known annotations does not solve problem of custom cyclic annotations as they are used in kotlin - f.e. kotlin.Metadata

…otation - pattern is the same as in isCDIEnablingAnnotation

svendiedrichsen · 2021-12-14T17:21:49Z

appserver/web/gf-weld-connector/src/main/java/org/glassfish/weld/connector/WeldUtils.java

@@ -403,20 +368,29 @@ private static boolean isCDIEnablingAnnotation(AnnotationType annotationType,
    public static boolean hasValidAnnotation(Class              annotatedClass,


Could this method signature be changed to accept validScopes and excludedScopes as Set<String> painlessly? This would make copying the collections obsolete.

There are placese where constant fields are passed, so we should not modify them. The copy is done on public api level and collections passed as parameter are only local implementation which is safe to verify their modification.

svendiedrichsen · 2021-12-14T17:24:50Z

...er/web/weld-integration/src/main/java/org/glassfish/weld/services/InjectionServicesImpl.java

@@ -95,8 +97,7 @@ public InjectionServicesImpl(InjectionManager injectionMgr, BundleDescriptor con
     * @return 
     */
    private boolean isInterceptor( Class beanClass ) {
-      HashSet<String> annos = new HashSet<>();
-      annos.add( javax.interceptor.Interceptor.class.getName() );
+      final Set<String> annos = Collections.singleton(javax.interceptor.Interceptor.class.getName());


This returns an immutable Set. Is this intentional? If you remove the collection copying from the other method modifications of this set will throw an exception.

I preffer to not modify collections passed as parameter to public API.

Sure. Good rule.

OK, only validScopes are not modified. There it is possible.

- so we can use set directly

svendiedrichsen · 2021-12-14T20:45:15Z

@sgflt LGTM. Good job.

breakponchito · 2021-12-16T15:28:48Z

Jenkins test

breakponchito · 2021-12-16T17:18:12Z

Jenkins test

breakponchito

LGTM

sgflt added 9 commits December 9, 2021 20:10

payara#5530 Stop scanning after first hit

463c965

payara#5530 Improved performance of getCDIEnablingAnnotations

de082a3

- removed redundant contains on set - list replaced by set - exclusions extracted because there is no need to discover the same annotation dozentimes and define it as excluded over and over again

payara#5530 Improved performance of getCDIAnnotatedClassNames

02ca980

- removed redundant contains on set - list replaced by set - exclusions extracted because there is no need to discover the same annotation dozentimes and define it as excluded over and over again

payara#5530 Share exclusions through all recursive iterations

926f539

payara#5530 Removed redundant condition

515d422

- if the annotation is cdi enabling, then it does not matter whether it is excluded or not

payara#5530 Moved cached set to the outer for

5e11bb5

- there we want to list all classes with annotation - if we exclude some custom annotation then only the first class with that annotation was included

payara#5530 Use precomputed set of cdi annotations

918708a

payara#5530 Break as soon as possible

1768af6

payara#5530 Updated copyright year

9dd2d60

svendiedrichsen reviewed Dec 12, 2021

View reviewed changes

sgflt marked this pull request as draft December 14, 2021 06:01

sgflt added 4 commits December 14, 2021 17:30

payara#5530 Replaced lists by sets

abd44db

- as they are used for heavy lookups

payara#5530 Deleted unused code

ab7d239

- this small subset of well known annotations does not solve problem of custom cyclic annotations as they are used in kotlin - f.e. kotlin.Metadata

payara#5530 Improved performance of hasValidAnnotation and isValidAnn…

3567e07

…otation - pattern is the same as in isCDIEnablingAnnotation

payara#5530 Moved condition to outer if

aa717e0

svendiedrichsen reviewed Dec 14, 2021

View reviewed changes

sgflt added 3 commits December 14, 2021 18:31

payara#5530 Simplified set creation

92cab66

payara#5530 validScopes are not modified

98d4b1d

- so we can use set directly

payara#5530 Allow only Sets as parameters

aa6b2df

sgflt marked this pull request as ready for review December 14, 2021 19:22

rdebusscher added the PR: CLA CLA submitted on PR by the contributor label Dec 17, 2021

breakponchito self-requested a review January 28, 2022 22:46

breakponchito approved these changes Jan 28, 2022

View reviewed changes

breakponchito merged commit 556dbad into payara:master Jan 28, 2022

Pandrex247 changed the title ~~5530 improve cdi scan~~ FISH-5938 5530 improve cdi scan Feb 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FISH-5938 5530 improve cdi scan #5531

FISH-5938 5530 improve cdi scan #5531

sgflt commented Dec 9, 2021

svendiedrichsen Dec 12, 2021 •

edited

Loading

sgflt Dec 14, 2021

svendiedrichsen commented Dec 12, 2021 •

edited

Loading

svendiedrichsen commented Dec 13, 2021 •

edited

Loading

svendiedrichsen Dec 14, 2021 •

edited

Loading

sgflt Dec 14, 2021

svendiedrichsen Dec 14, 2021

svendiedrichsen Dec 14, 2021 •

edited

Loading

sgflt Dec 14, 2021

svendiedrichsen Dec 14, 2021

sgflt Dec 14, 2021

svendiedrichsen commented Dec 14, 2021

breakponchito commented Dec 16, 2021

breakponchito commented Dec 16, 2021

breakponchito left a comment

		@@ -403,20 +368,29 @@ private static boolean isCDIEnablingAnnotation(AnnotationType annotationType,
		public static boolean hasValidAnnotation(Class annotatedClass,

FISH-5938 5530 improve cdi scan #5531

FISH-5938 5530 improve cdi scan #5531

Conversation

sgflt commented Dec 9, 2021

Testing Performed

svendiedrichsen Dec 12, 2021 • edited Loading

Choose a reason for hiding this comment

sgflt Dec 14, 2021

Choose a reason for hiding this comment

svendiedrichsen commented Dec 12, 2021 • edited Loading

svendiedrichsen commented Dec 13, 2021 • edited Loading

svendiedrichsen Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

sgflt Dec 14, 2021

Choose a reason for hiding this comment

svendiedrichsen Dec 14, 2021

Choose a reason for hiding this comment

svendiedrichsen Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

sgflt Dec 14, 2021

Choose a reason for hiding this comment

svendiedrichsen Dec 14, 2021

Choose a reason for hiding this comment

sgflt Dec 14, 2021

Choose a reason for hiding this comment

svendiedrichsen commented Dec 14, 2021

breakponchito commented Dec 16, 2021

breakponchito commented Dec 16, 2021

breakponchito left a comment

Choose a reason for hiding this comment

svendiedrichsen Dec 12, 2021 •

edited

Loading

svendiedrichsen commented Dec 12, 2021 •

edited

Loading

svendiedrichsen commented Dec 13, 2021 •

edited

Loading

svendiedrichsen Dec 14, 2021 •

edited

Loading

svendiedrichsen Dec 14, 2021 •

edited

Loading