[GitHub] [tomcat] kamnani opened a new pull request #354: Optimize Server startup time using multi-threading for annotation scanning

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
32 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani opened a new pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox

kamnani opened a new pull request #354:
URL: https://github.com/apache/tomcat/pull/354


   This is a redo of Previous PR: https://github.com/apache/tomcat/pull/349
   
   The following changes have been made based on the suggestions earlier:
   1) Flags (parallelAnnotationScanning & maxThreadsForParallelAnnotationScanning) can be passed through Host Configuration. See example below.
   2) By default parallelAnnotationScanning remains disabled and thus default tomcat behavior is untouched.
   3) maxThreadsForParallelAnnotationScanning defaults to 1 and will be in-effect if parallelAnnotationScanning is enabled via Host Configs.
   3) PR is against master.
   
   Apologies in case something is missed.
   
   ```
   <Host name="localhost"  appBase="webapps"
               unpackWARs="true" autoDeploy="true" parallelAnnotationScanning="false" maxThreadsForParallelAnnotationScanning="10">


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] rmaucher commented on pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox

rmaucher commented on pull request #354:
URL: https://github.com/apache/tomcat/pull/354#issuecomment-687293425


   I don't agree with this. There is a "utility" executor (Server.getUtilityExecutor()), it could be used for that instead of adding special purpose threads. Then you can add a flag to do the scanning as executor tasks or the usual way.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

kamnani commented on pull request #354:
URL: https://github.com/apache/tomcat/pull/354#issuecomment-687380392


   Thanks @rmaucher for your comment. I have made the necessary changes as suggested. It would be great if you could verify that change. I have also updated the PR description since the flags have now been changed with this change.
   Is there any other change needed on this PR you want to suggest ?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani edited a comment on pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

kamnani edited a comment on pull request #354:
URL: https://github.com/apache/tomcat/pull/354#issuecomment-687380392


   Thanks @rmaucher for your comment. I have made the necessary changes as suggested. It would be great if you could verify that change. I have also updated the PR description since the flags required to enable this have now been changed with this commit.
   Is there any other change needed on this PR you want to suggest ?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] rmaucher commented on pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

rmaucher commented on pull request #354:
URL: https://github.com/apache/tomcat/pull/354#issuecomment-687873084


   I still don't really like some items, like the explicit flag (but I don't see yet how to do it) or some of the "optimizations".
   By default the utility thread count is 2, so not too much parallelism.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] rmaucher commented on a change in pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

rmaucher commented on a change in pull request #354:
URL: https://github.com/apache/tomcat/pull/354#discussion_r484426457



##########
File path: java/org/apache/catalina/startup/ContextConfig.java
##########
@@ -122,6 +126,11 @@
 
     private static final Log log = LogFactory.getLog(ContextConfig.class);
 
+    private static final int DEFAULT_CLASS_CACHE_SIZE = 16384;
+
+    private static final float DEFAULT_LOAD_FACTOR = .75f;
+
+    private static int CONCURRENCY_LEVEL = 1;

Review comment:
       Ok for the map size, maybe (it seems very large, 16k classes ?), I don't quite see the point of the rest. The concurrency may be up to the utility executor thread count, and I don't think this is an optimization which makes any difference.

##########
File path: java/org/apache/catalina/startup/ContextConfig.java
##########
@@ -1374,7 +1383,19 @@ protected void webConfig() {
     protected void processClasses(WebXml webXml, Set<WebXml> orderedFragments) {
         // Step 4. Process /WEB-INF/classes for annotations and
         // @HandlesTypes matches
-        Map<String, JavaClassCacheEntry> javaClassCache = new HashMap<>();
+
+        Map<String, JavaClassCacheEntry> javaClassCache;
+
+        if (context.getParent() instanceof Host) {
+           Host host = (Host) context.getParent();
+            Container container = host.getParent();
+            CONCURRENCY_LEVEL = container.getStartStopThreads();
+            javaClassCache = new ConcurrentHashMap<>(DEFAULT_CLASS_CACHE_SIZE, DEFAULT_LOAD_FACTOR,
+                    CONCURRENCY_LEVEL);
+        } else {
+            javaClassCache = new ConcurrentHashMap<>(DEFAULT_CLASS_CACHE_SIZE, DEFAULT_LOAD_FACTOR,
+                    CONCURRENCY_LEVEL);
+        }

Review comment:
       Well, if it's changed to be always a ConcurrentHashMap, it probably won't be a measurable performance difference.

##########
File path: java/org/apache/catalina/startup/ContextConfig.java
##########
@@ -2136,26 +2157,98 @@ protected InputSource getWebXmlSource(String filename, boolean global) {
     }
 
     protected void processAnnotations(Set<WebXml> fragments,
-            boolean handlesTypesOnly, Map<String,JavaClassCacheEntry> javaClassCache) {
-        for(WebXml fragment : fragments) {
-            // Only need to scan for @HandlesTypes matches if any of the
-            // following are true:
-            // - it has already been determined only @HandlesTypes is required
-            //   (e.g. main web.xml has metadata-complete="true"
-            // - this fragment is for a container JAR (Servlet 3.1 section 8.1)
-            // - this fragment has metadata-complete="true"
-            boolean htOnly = handlesTypesOnly || !fragment.getWebappJar() ||
-                    fragment.isMetadataComplete();
-
-            WebXml annotations = new WebXml();
-            // no impact on distributable
-            annotations.setDistributable(true);
-            URL url = fragment.getURL();
-            processAnnotationsUrl(url, annotations, htOnly, javaClassCache);
-            Set<WebXml> set = new HashSet<>();
-            set.add(annotations);
-            // Merge annotations into fragment - fragment takes priority
-            fragment.merge(set);
+            boolean handlesTypesOnly, Map<String, JavaClassCacheEntry> javaClassCache) {
+
+        if (context.getParent() instanceof Host && ((Host) context.getParent()).isParallelAnnotationScanning()) {
+            processAnnotationsInParallel(fragments, handlesTypesOnly, javaClassCache);
+            return;
+        }
+
+        for (WebXml fragment : fragments) {
+            scanWebXmlFragment(handlesTypesOnly, fragment, javaClassCache);
+        }
+    }
+
+    private void scanWebXmlFragment(boolean handlesTypesOnly, WebXml fragment, Map<String, JavaClassCacheEntry> javaClassCache) {
+
+        // Only need to scan for @HandlesTypes matches if any of the
+        // following are true:
+        // - it has already been determined only @HandlesTypes is required
+        //   (e.g. main web.xml has metadata-complete="true"
+        // - this fragment is for a container JAR (Servlet 3.1 section 8.1)
+        // - this fragment has metadata-complete="true"
+        boolean htOnly = handlesTypesOnly || !fragment.getWebappJar() ||
+                fragment.isMetadataComplete();
+
+        WebXml annotations = new WebXml();
+        // no impact on distributable
+        annotations.setDistributable(true);
+        URL url = fragment.getURL();
+        processAnnotationsUrl(url, annotations, htOnly, javaClassCache);
+        Set<WebXml> set = new HashSet<>();
+        set.add(annotations);
+        // Merge annotations into fragment - fragment takes priority
+        fragment.merge(set);
+    }
+
+    /**
+     * Executable task to scan a segment for annotations. Each task does the
+     * same work as the for loop inside processAnnotations();
+     *
+     * @author Engebretson, John
+     * @author Kamnani, Jatin
+     */
+    private class AnnotationScanTask implements Callable<Void> {
+        private final WebXml fragment;
+        private final boolean handlesTypesOnly;
+        private Map<String, JavaClassCacheEntry> javaClassCache;
+
+        private AnnotationScanTask(WebXml fragment, boolean handlesTypesOnly, Map<String, JavaClassCacheEntry> javaClassCache) {
+            this.fragment = fragment;
+            this.handlesTypesOnly = handlesTypesOnly;
+            this.javaClassCache = javaClassCache;
+        }
+
+        @Override
+        public Void call() {
+            scanWebXmlFragment(handlesTypesOnly, fragment, javaClassCache);
+
+            return null;
+        }
+
+    }
+
+    /**
+     * Parallelized version of processAnnotationsInParallel(). Constructs tasks,
+     * submits them as they're created, then waits for completion.
+     *
+     * @param fragments        Set of parallelizable scans
+     * @param handlesTypesOnly Important parameter for the underlying scan
+     */
+    protected void processAnnotationsInParallel(Set<WebXml> fragments, boolean handlesTypesOnly,
+                                                Map<String, JavaClassCacheEntry> javaClassCache) {
+
+
+        Server s = getServer();
+        ExecutorService pool = null;
+        try {
+            pool = s.getUtilityExecutor();
+            List<Future<Void>> futures = new ArrayList<>(fragments.size());
+            for (WebXml fragment : fragments) {
+                Callable<Void> task = new AnnotationScanTask(fragment, handlesTypesOnly, javaClassCache);
+                futures.add(pool.submit(task));
+            }
+            try {
+                for (Future<?> future : futures) {
+                    future.get();
+                }
+            } catch (Exception e) {
+                throw new RuntimeException("Parallel execution failed", e);

Review comment:
       Localization missing.

##########
File path: java/org/apache/catalina/mbeans/MBeanFactory.java
##########
@@ -490,6 +491,7 @@ public String createStandardContext(String parent,
     public String createStandardHost(String parent, String name,
                                      String appBase,
                                      boolean autoDeploy,
+                                     boolean parallelAnnotationScanning,

Review comment:
       I disagree with this API change. IMO this new feature does not justify it.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

kamnani commented on pull request #354:
URL: https://github.com/apache/tomcat/pull/354#issuecomment-688967898


   > I still don't really like some items, like the explicit flag (but I don't see yet how to do it) or some of the "optimizations".
   > By default the utility thread count is 2, so not too much parallelism.
   
   Yes, You're right with the default Value, but that can be changed through the Host Configuration.. right?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani edited a comment on pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

kamnani edited a comment on pull request #354:
URL: https://github.com/apache/tomcat/pull/354#issuecomment-688967898


   > I still don't really like some items, like the explicit flag (but I don't see yet how to do it) or some of the "optimizations".
   > By default the utility thread count is 2, so not too much parallelism.
   
   Yes, You're right with the default value, but that can be changed through the Host Configuration.. right?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #354:
URL: https://github.com/apache/tomcat/pull/354#discussion_r485071970



##########
File path: java/org/apache/catalina/startup/ContextConfig.java
##########
@@ -2136,26 +2157,98 @@ protected InputSource getWebXmlSource(String filename, boolean global) {
     }
 
     protected void processAnnotations(Set<WebXml> fragments,
-            boolean handlesTypesOnly, Map<String,JavaClassCacheEntry> javaClassCache) {
-        for(WebXml fragment : fragments) {
-            // Only need to scan for @HandlesTypes matches if any of the
-            // following are true:
-            // - it has already been determined only @HandlesTypes is required
-            //   (e.g. main web.xml has metadata-complete="true"
-            // - this fragment is for a container JAR (Servlet 3.1 section 8.1)
-            // - this fragment has metadata-complete="true"
-            boolean htOnly = handlesTypesOnly || !fragment.getWebappJar() ||
-                    fragment.isMetadataComplete();
-
-            WebXml annotations = new WebXml();
-            // no impact on distributable
-            annotations.setDistributable(true);
-            URL url = fragment.getURL();
-            processAnnotationsUrl(url, annotations, htOnly, javaClassCache);
-            Set<WebXml> set = new HashSet<>();
-            set.add(annotations);
-            // Merge annotations into fragment - fragment takes priority
-            fragment.merge(set);
+            boolean handlesTypesOnly, Map<String, JavaClassCacheEntry> javaClassCache) {
+
+        if (context.getParent() instanceof Host && ((Host) context.getParent()).isParallelAnnotationScanning()) {
+            processAnnotationsInParallel(fragments, handlesTypesOnly, javaClassCache);
+            return;
+        }
+
+        for (WebXml fragment : fragments) {
+            scanWebXmlFragment(handlesTypesOnly, fragment, javaClassCache);
+        }
+    }
+
+    private void scanWebXmlFragment(boolean handlesTypesOnly, WebXml fragment, Map<String, JavaClassCacheEntry> javaClassCache) {
+
+        // Only need to scan for @HandlesTypes matches if any of the
+        // following are true:
+        // - it has already been determined only @HandlesTypes is required
+        //   (e.g. main web.xml has metadata-complete="true"
+        // - this fragment is for a container JAR (Servlet 3.1 section 8.1)
+        // - this fragment has metadata-complete="true"
+        boolean htOnly = handlesTypesOnly || !fragment.getWebappJar() ||
+                fragment.isMetadataComplete();
+
+        WebXml annotations = new WebXml();
+        // no impact on distributable
+        annotations.setDistributable(true);
+        URL url = fragment.getURL();
+        processAnnotationsUrl(url, annotations, htOnly, javaClassCache);
+        Set<WebXml> set = new HashSet<>();
+        set.add(annotations);
+        // Merge annotations into fragment - fragment takes priority
+        fragment.merge(set);
+    }
+
+    /**
+     * Executable task to scan a segment for annotations. Each task does the
+     * same work as the for loop inside processAnnotations();
+     *
+     * @author Engebretson, John
+     * @author Kamnani, Jatin
+     */
+    private class AnnotationScanTask implements Callable<Void> {
+        private final WebXml fragment;
+        private final boolean handlesTypesOnly;
+        private Map<String, JavaClassCacheEntry> javaClassCache;
+
+        private AnnotationScanTask(WebXml fragment, boolean handlesTypesOnly, Map<String, JavaClassCacheEntry> javaClassCache) {
+            this.fragment = fragment;
+            this.handlesTypesOnly = handlesTypesOnly;
+            this.javaClassCache = javaClassCache;
+        }
+
+        @Override
+        public Void call() {
+            scanWebXmlFragment(handlesTypesOnly, fragment, javaClassCache);
+
+            return null;
+        }
+
+    }
+
+    /**
+     * Parallelized version of processAnnotationsInParallel(). Constructs tasks,
+     * submits them as they're created, then waits for completion.
+     *
+     * @param fragments        Set of parallelizable scans
+     * @param handlesTypesOnly Important parameter for the underlying scan
+     */
+    protected void processAnnotationsInParallel(Set<WebXml> fragments, boolean handlesTypesOnly,
+                                                Map<String, JavaClassCacheEntry> javaClassCache) {
+
+
+        Server s = getServer();
+        ExecutorService pool = null;
+        try {
+            pool = s.getUtilityExecutor();
+            List<Future<Void>> futures = new ArrayList<>(fragments.size());
+            for (WebXml fragment : fragments) {
+                Callable<Void> task = new AnnotationScanTask(fragment, handlesTypesOnly, javaClassCache);
+                futures.add(pool.submit(task));
+            }
+            try {
+                for (Future<?> future : futures) {
+                    future.get();
+                }
+            } catch (Exception e) {
+                throw new RuntimeException("Parallel execution failed", e);

Review comment:
       Resolved with the latest changes.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #354:
URL: https://github.com/apache/tomcat/pull/354#discussion_r485072258



##########
File path: java/org/apache/catalina/startup/ContextConfig.java
##########
@@ -122,6 +126,11 @@
 
     private static final Log log = LogFactory.getLog(ContextConfig.class);
 
+    private static final int DEFAULT_CLASS_CACHE_SIZE = 16384;
+
+    private static final float DEFAULT_LOAD_FACTOR = .75f;
+
+    private static int CONCURRENCY_LEVEL = 1;

Review comment:
       Resolved with latest commit.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #354:
URL: https://github.com/apache/tomcat/pull/354#discussion_r485073735



##########
File path: java/org/apache/catalina/startup/ContextConfig.java
##########
@@ -1374,7 +1383,19 @@ protected void webConfig() {
     protected void processClasses(WebXml webXml, Set<WebXml> orderedFragments) {
         // Step 4. Process /WEB-INF/classes for annotations and
         // @HandlesTypes matches
-        Map<String, JavaClassCacheEntry> javaClassCache = new HashMap<>();
+
+        Map<String, JavaClassCacheEntry> javaClassCache;
+
+        if (context.getParent() instanceof Host) {
+           Host host = (Host) context.getParent();
+            Container container = host.getParent();
+            CONCURRENCY_LEVEL = container.getStartStopThreads();
+            javaClassCache = new ConcurrentHashMap<>(DEFAULT_CLASS_CACHE_SIZE, DEFAULT_LOAD_FACTOR,
+                    CONCURRENCY_LEVEL);
+        } else {
+            javaClassCache = new ConcurrentHashMap<>(DEFAULT_CLASS_CACHE_SIZE, DEFAULT_LOAD_FACTOR,
+                    CONCURRENCY_LEVEL);
+        }

Review comment:
       Have added the default behavior without the flag in the latest commit.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

kamnani commented on pull request #354:
URL: https://github.com/apache/tomcat/pull/354#issuecomment-689023592


   @rmaucher I actually tested the optimization with the flag in a large application having 1000's of Jar in the classpath and the server startup gains are significant and the stats are as follows :
   2816 classes -  44% improvement
   1800 classes - 40% improvement
   1000 classes - 40% improvement
   
   The default tomcat app shows an improvement of 6% with the flag.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani edited a comment on pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

kamnani edited a comment on pull request #354:
URL: https://github.com/apache/tomcat/pull/354#issuecomment-689023592


   @rmaucher I actually tested the optimization with the flag in a large application having 1000's of Jar in the classpath and the server startup gains are significant and the stats are as follows :
   2816 jars -  44% improvement
   1800 jars - 40% improvement
   1000 jars - 40% improvement
   
   The default tomcat app shows an improvement of 6% with the flag.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #354:
URL: https://github.com/apache/tomcat/pull/354#discussion_r485091168



##########
File path: java/org/apache/catalina/mbeans/MBeanFactory.java
##########
@@ -490,6 +491,7 @@ public String createStandardContext(String parent,
     public String createStandardHost(String parent, String name,
                                      String appBase,
                                      boolean autoDeploy,
+                                     boolean parallelAnnotationScanning,

Review comment:
       Thanks @rmaucher for the comment.
   I have little experience working on Tomcat so if you can you explain how this becomes irrelevant that would be great. Also, do we not want the API's to be able to pass this flag when the optimizations are quite significant?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani edited a comment on pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

kamnani edited a comment on pull request #354:
URL: https://github.com/apache/tomcat/pull/354#issuecomment-689023592


   @rmaucher I actually tested the optimization with the flag in a large application having 1000's of Jar in the classpath and the server startup gains are significant and the stats are as follows :
   2816 jars -  33% improvement
   1800 jars - 30% improvement
   1000 jars - 30% improvement
   
   The default tomcat app shows an improvement of 6% with the flag.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] rmaucher commented on a change in pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

rmaucher commented on a change in pull request #354:
URL: https://github.com/apache/tomcat/pull/354#discussion_r485121593



##########
File path: java/org/apache/catalina/mbeans/MBeanFactory.java
##########
@@ -490,6 +491,7 @@ public String createStandardContext(String parent,
     public String createStandardHost(String parent, String name,
                                      String appBase,
                                      boolean autoDeploy,
+                                     boolean parallelAnnotationScanning,

Review comment:
       This is not enough to justify changing this API. You can create the host, then set the flag just after. Good enough.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] kamnani commented on a change in pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

kamnani commented on a change in pull request #354:
URL: https://github.com/apache/tomcat/pull/354#discussion_r485129011



##########
File path: java/org/apache/catalina/mbeans/MBeanFactory.java
##########
@@ -490,6 +491,7 @@ public String createStandardContext(String parent,
     public String createStandardHost(String parent, String name,
                                      String appBase,
                                      boolean autoDeploy,
+                                     boolean parallelAnnotationScanning,

Review comment:
       Alright. I have removed these changes.
   Resolved with the latest commit.
   Thanks đź‘Ť




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] martin-g commented on a change in pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

martin-g commented on a change in pull request #354:
URL: https://github.com/apache/tomcat/pull/354#discussion_r486870063



##########
File path: java/org/apache/catalina/startup/LocalStrings_fr.properties
##########
@@ -69,6 +69,7 @@ contextConfig.missingRealm=Aucun royaume (realm) n'a été configuré pour réal
 contextConfig.processAnnotationsDir.debug=Balayage du rĂ©pertoire pour trouver des fichiers de classe avec annotations [{0}]
 contextConfig.processAnnotationsJar.debug=Analyse du fichier jars pour des classes annotĂ©es avec [{0}]
 contextConfig.processAnnotationsWebDir.debug=Balayage du rĂ©pertoire d''applications web, pour fichiers de classe avec annotations [{0}]
+contextConfig.processAnnotationsInParallelFailure=exécution parallèle a échoué

Review comment:
       Should `exĂ©cution` be capital cased like the other messages ?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] martin-g commented on a change in pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

martin-g commented on a change in pull request #354:
URL: https://github.com/apache/tomcat/pull/354#discussion_r486872606



##########
File path: java/org/apache/catalina/startup/ContextConfig.java
##########
@@ -2136,26 +2146,98 @@ protected InputSource getWebXmlSource(String filename, boolean global) {
     }
 
     protected void processAnnotations(Set<WebXml> fragments,
-            boolean handlesTypesOnly, Map<String,JavaClassCacheEntry> javaClassCache) {
-        for(WebXml fragment : fragments) {
-            // Only need to scan for @HandlesTypes matches if any of the
-            // following are true:
-            // - it has already been determined only @HandlesTypes is required
-            //   (e.g. main web.xml has metadata-complete="true"
-            // - this fragment is for a container JAR (Servlet 3.1 section 8.1)
-            // - this fragment has metadata-complete="true"
-            boolean htOnly = handlesTypesOnly || !fragment.getWebappJar() ||
-                    fragment.isMetadataComplete();
-
-            WebXml annotations = new WebXml();
-            // no impact on distributable
-            annotations.setDistributable(true);
-            URL url = fragment.getURL();
-            processAnnotationsUrl(url, annotations, htOnly, javaClassCache);
-            Set<WebXml> set = new HashSet<>();
-            set.add(annotations);
-            // Merge annotations into fragment - fragment takes priority
-            fragment.merge(set);
+            boolean handlesTypesOnly, Map<String, JavaClassCacheEntry> javaClassCache) {
+
+        if (context.getParent() instanceof Host && ((Host) context.getParent()).isParallelAnnotationScanning()) {
+            processAnnotationsInParallel(fragments, handlesTypesOnly, javaClassCache);
+            return;
+        }
+
+        for (WebXml fragment : fragments) {
+            scanWebXmlFragment(handlesTypesOnly, fragment, javaClassCache);
+        }
+    }
+
+    private void scanWebXmlFragment(boolean handlesTypesOnly, WebXml fragment, Map<String, JavaClassCacheEntry> javaClassCache) {
+
+        // Only need to scan for @HandlesTypes matches if any of the
+        // following are true:
+        // - it has already been determined only @HandlesTypes is required
+        //   (e.g. main web.xml has metadata-complete="true"
+        // - this fragment is for a container JAR (Servlet 3.1 section 8.1)
+        // - this fragment has metadata-complete="true"
+        boolean htOnly = handlesTypesOnly || !fragment.getWebappJar() ||
+                fragment.isMetadataComplete();
+
+        WebXml annotations = new WebXml();
+        // no impact on distributable
+        annotations.setDistributable(true);
+        URL url = fragment.getURL();
+        processAnnotationsUrl(url, annotations, htOnly, javaClassCache);
+        Set<WebXml> set = new HashSet<>();
+        set.add(annotations);
+        // Merge annotations into fragment - fragment takes priority
+        fragment.merge(set);
+    }
+
+    /**
+     * Executable task to scan a segment for annotations. Each task does the
+     * same work as the for loop inside processAnnotations();
+     *
+     * @author Engebretson, John
+     * @author Kamnani, Jatin
+     */
+    private class AnnotationScanTask implements Callable<Void> {
+        private final WebXml fragment;
+        private final boolean handlesTypesOnly;
+        private Map<String, JavaClassCacheEntry> javaClassCache;
+
+        private AnnotationScanTask(WebXml fragment, boolean handlesTypesOnly, Map<String, JavaClassCacheEntry> javaClassCache) {
+            this.fragment = fragment;
+            this.handlesTypesOnly = handlesTypesOnly;
+            this.javaClassCache = javaClassCache;
+        }
+
+        @Override
+        public Void call() {
+            scanWebXmlFragment(handlesTypesOnly, fragment, javaClassCache);
+
+            return null;
+        }
+
+    }
+
+    /**
+     * Parallelized version of processAnnotationsInParallel(). Constructs tasks,
+     * submits them as they're created, then waits for completion.
+     *
+     * @param fragments        Set of parallelizable scans
+     * @param handlesTypesOnly Important parameter for the underlying scan
+     */
+    protected void processAnnotationsInParallel(Set<WebXml> fragments, boolean handlesTypesOnly,
+                                                Map<String, JavaClassCacheEntry> javaClassCache) {
+
+
+        Server s = getServer();
+        ExecutorService pool = null;
+        try {
+            pool = s.getUtilityExecutor();
+            List<Future<Void>> futures = new ArrayList<>(fragments.size());
+            for (WebXml fragment : fragments) {
+                Callable<Void> task = new AnnotationScanTask(fragment, handlesTypesOnly, javaClassCache);
+                futures.add(pool.submit(task));
+            }
+            try {
+                for (Future<?> future : futures) {
+                    future.get();
+                }
+            } catch (Exception e) {
+                throw new RuntimeException(sm.getString("contextConfig.processAnnotationsInParallelFailure"), e);
+            }
+        } finally {
+            if (pool != null) {
+                pool.shutdownNow();

Review comment:
       This is not really needed because org.apache.tomcat.util.threads.ScheduledThreadPoolExecutor#shutdown methods do nothing, but anyway it is a good practice to do it.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] martin-g commented on a change in pull request #354: Optimize Server startup time using multi-threading for annotation scanning

GitBox
In reply to this post by GitBox

martin-g commented on a change in pull request #354:
URL: https://github.com/apache/tomcat/pull/354#discussion_r486875285



##########
File path: java/org/apache/catalina/startup/ContextConfig.java
##########
@@ -2136,26 +2146,98 @@ protected InputSource getWebXmlSource(String filename, boolean global) {
     }
 
     protected void processAnnotations(Set<WebXml> fragments,
-            boolean handlesTypesOnly, Map<String,JavaClassCacheEntry> javaClassCache) {
-        for(WebXml fragment : fragments) {
-            // Only need to scan for @HandlesTypes matches if any of the
-            // following are true:
-            // - it has already been determined only @HandlesTypes is required
-            //   (e.g. main web.xml has metadata-complete="true"
-            // - this fragment is for a container JAR (Servlet 3.1 section 8.1)
-            // - this fragment has metadata-complete="true"
-            boolean htOnly = handlesTypesOnly || !fragment.getWebappJar() ||
-                    fragment.isMetadataComplete();
-
-            WebXml annotations = new WebXml();
-            // no impact on distributable
-            annotations.setDistributable(true);
-            URL url = fragment.getURL();
-            processAnnotationsUrl(url, annotations, htOnly, javaClassCache);
-            Set<WebXml> set = new HashSet<>();
-            set.add(annotations);
-            // Merge annotations into fragment - fragment takes priority
-            fragment.merge(set);
+            boolean handlesTypesOnly, Map<String, JavaClassCacheEntry> javaClassCache) {
+
+        if (context.getParent() instanceof Host && ((Host) context.getParent()).isParallelAnnotationScanning()) {
+            processAnnotationsInParallel(fragments, handlesTypesOnly, javaClassCache);
+            return;
+        }
+
+        for (WebXml fragment : fragments) {
+            scanWebXmlFragment(handlesTypesOnly, fragment, javaClassCache);
+        }
+    }
+
+    private void scanWebXmlFragment(boolean handlesTypesOnly, WebXml fragment, Map<String, JavaClassCacheEntry> javaClassCache) {
+
+        // Only need to scan for @HandlesTypes matches if any of the
+        // following are true:
+        // - it has already been determined only @HandlesTypes is required
+        //   (e.g. main web.xml has metadata-complete="true"
+        // - this fragment is for a container JAR (Servlet 3.1 section 8.1)
+        // - this fragment has metadata-complete="true"
+        boolean htOnly = handlesTypesOnly || !fragment.getWebappJar() ||
+                fragment.isMetadataComplete();
+
+        WebXml annotations = new WebXml();
+        // no impact on distributable
+        annotations.setDistributable(true);
+        URL url = fragment.getURL();
+        processAnnotationsUrl(url, annotations, htOnly, javaClassCache);
+        Set<WebXml> set = new HashSet<>();
+        set.add(annotations);
+        // Merge annotations into fragment - fragment takes priority
+        fragment.merge(set);
+    }
+
+    /**
+     * Executable task to scan a segment for annotations. Each task does the
+     * same work as the for loop inside processAnnotations();
+     *
+     * @author Engebretson, John
+     * @author Kamnani, Jatin
+     */
+    private class AnnotationScanTask implements Callable<Void> {

Review comment:
       Why `Callable<Void>` ?
   You can use `Runnable`




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

12