[JENKINS-17363] Ludicrously slow load time [with lazyloading]

[JENKINS-19437] Implement load on demand functionality in Cppcheck - Lazy loaded details are stored in CppcheckResult member variable and never released by Java garbace collector since a reference to them exists forever. All build.xml files are loaded during Jenkins startup and never released, CppcheckResult objects are part of them. This is kind of a memory leak. The update will cause slight lower performance but releasing of the memory is much more important for long run tasks/daemons/servers. - Method lazyLoadSourceContainer() updated to return the data instead of their caching in a member variable. - If cached CppcheckSourceContainer object can't be loaded, an empty one will be created. Different constructor used to remove unnecessary exception catches. Recently added related if removed from CppcheckSourceContainer. - Condition moved from index.jelly to summary.jelly to be able to reuse the data and remove one unnecessary loading (performance). - Construction "key = ++key" has no meaning, updated to "++key" to resolve a warning.
jenkinsci · Feb 22, 2014 · 5db2c79 · 5db2c79
1 parent e912256
commit 5db2c79
Show file tree

Hide file tree

Showing 4 changed files with 61 additions and 67 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/cppcheck/CppcheckResult.java b/src/main/java/org/jenkinsci/plugins/cppcheck/CppcheckResult.java
@@ -1,7 +1,6 @@
 package org.jenkinsci.plugins.cppcheck;
 
 import com.thalesgroup.hudson.plugins.cppcheck.CppcheckSource;
-import com.thalesgroup.hudson.plugins.cppcheck.model.CppcheckFile;
 import com.thalesgroup.hudson.plugins.cppcheck.model.CppcheckWorkspaceFile;
 
 import hudson.XmlFile;
@@ -18,8 +17,8 @@
 import java.io.File;
 import java.io.IOException;
 import java.io.Serializable;
-import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.Map;
 
 /**
@@ -111,8 +110,7 @@ public CppcheckStatistics getStatistics() {
     }
 
     public CppcheckSourceContainer getCppcheckSourceContainer() {
-        lazyLoadSourceContainer();
-        return cppcheckSourceContainer;
+        return lazyLoadSourceContainer();
     }
 
     /**
@@ -307,26 +305,22 @@ private Object readResolve() {
 
     /**
      * Lazy load source container data if they are not already loaded.
+     * 
+     * @return the loaded and parsed data or empty object on error
      */
-    private void lazyLoadSourceContainer() {
+    private CppcheckSourceContainer lazyLoadSourceContainer() {
+        // Backward compatibility with version 1.14 and less
         if(cppcheckSourceContainer != null) {
-            return;
+            return cppcheckSourceContainer;
         }
 
         XmlFile xmlSourceContainer = new XmlFile(new File(owner.getRootDir(),
                 CppcheckPublisher.XML_FILE_DETAILS));
         try {
-            cppcheckSourceContainer = (CppcheckSourceContainer) xmlSourceContainer.read();
+            return (CppcheckSourceContainer) xmlSourceContainer.read();
         } catch (IOException e) {
-            try {
-                cppcheckSourceContainer = new CppcheckSourceContainer(null,
-                        owner.getWorkspace(), owner.getModuleRoot(),
-                        new ArrayList<CppcheckFile>());
-            } catch (IOException e1) {
-                // Ignore, there is nothing to do
-            } catch (InterruptedException e1) {
-                // Ignore, there is nothing to do
-            }
+            return new CppcheckSourceContainer(new HashMap<Integer,
+                    CppcheckWorkspaceFile>());
         }
     }
 }
diff --git a/src/main/java/org/jenkinsci/plugins/cppcheck/CppcheckSourceContainer.java b/src/main/java/org/jenkinsci/plugins/cppcheck/CppcheckSourceContainer.java
@@ -34,7 +34,7 @@ public CppcheckSourceContainer(BuildListener listener,
             cppcheckFile.setKey(key);
             cppcheckWorkspaceFile.setCppcheckFile(cppcheckFile);
             internalMap.put(key, cppcheckWorkspaceFile);
-            key = ++key;
+            ++key;
         }
     }
 
@@ -55,10 +55,7 @@ private CppcheckWorkspaceFile getCppcheckWorkspaceFile(BuildListener listener,
         CppcheckWorkspaceFile cppcheckWorkspaceFile = new CppcheckWorkspaceFile();
         FilePath sourceFilePath = getSourceFile(workspace, scmRootDir, cppcheckFileName);
         if (!sourceFilePath.exists()) {
-            if(listener != null) {
-                CppcheckLogger.log(listener, "[WARNING] - The source file '" + sourceFilePath.toURI() + "' doesn't exist on the slave. The ability to display its source code has been removed.");
-            }
-
+            CppcheckLogger.log(listener, "[WARNING] - The source file '" + sourceFilePath.toURI() + "' doesn't exist on the slave. The ability to display its source code has been removed.");
             cppcheckWorkspaceFile.setFileName(null);
             cppcheckWorkspaceFile.setSourceIgnored(true);
         } else if (sourceFilePath.isDirectory()) {

diff --git a/src/main/resources/org/jenkinsci/plugins/cppcheck/CppcheckResult/index.jelly b/src/main/resources/org/jenkinsci/plugins/cppcheck/CppcheckResult/index.jelly
@@ -57,10 +57,7 @@
             </j:if>
 
             <j:if test="${it.statistics.numberTotal != 0}">
-                <j:if test="${it.cppcheckSourceContainer != null}">
-                    <h2>Details</h2>
-                    <st:include page="summary.jelly"/>
-                </j:if>
+                <st:include page="summary.jelly"/>
             </j:if>
         </l:main-panel>
     </l:layout>

diff --git a/src/main/resources/org/jenkinsci/plugins/cppcheck/CppcheckResult/summary.jelly b/src/main/resources/org/jenkinsci/plugins/cppcheck/CppcheckResult/summary.jelly
@@ -1,45 +1,51 @@
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:g="/jelly/cppcheck">
     <st:header name="Content-Type" value="text/html;charset=UTF-8"/>
-    <table class="pane sortable" id="files">
-        <tr>
-            <td class="pane-header">Filename</td>
-            <td class="pane-header" style="width:2em">LineNumber</td>
-            <td class="pane-header" style="width:2em">CppCheckId</td>
-            <td class="pane-header" style="width:2em">Severity</td>
-            <td class="pane-header" style="width:60%">Message</td>
-        </tr>
-        <tbody>
-
-            <j:forEach var="elt" items="${it.cppcheckSourceContainer.internalMap.values()}">
-                <j:set var="cppcheckfile" value="${elt.cppcheckFile}"/>
-                <tr>
-                    <td class="pane">
-                        <j:if test="${elt.isSourceIgnored()}">
-                            ${cppcheckfile.fileName}
-                        </j:if>
-                        <j:if test="${not elt.isSourceIgnored()}">
-                            <a href="source.${cppcheckfile.key}">${cppcheckfile.fileName}</a>
-                        </j:if>
-
-
-                    </td>
-                    <td class="pane">
-                        <j:if
-                                test="${elt.isSourceIgnored()}">
-                            ${cppcheckfile.lineNumber}
-                        </j:if>
-                        <j:if test="${not elt.isSourceIgnored()}">
-                            <a href="source.${cppcheckfile.key}#${cppcheckfile.lineNumber}">${cppcheckfile.lineNumber}
-                            </a>
-                        </j:if>
-                    </td>
-                    <g:format value="${cppcheckfile.cppCheckId}" severity="${cppcheckfile.severity}"/>
-                    <g:format value="${cppcheckfile.severity}" severity="${cppcheckfile.severity}"/>
-                    <g:format value="${cppcheckfile.message}" severity="${cppcheckfile.severity}"/>
-                </tr>
-            </j:forEach>
-
-
-        </tbody>
-    </table>
+
+    <j:set var="cachedContainer" value="${it.cppcheckSourceContainer}"/>
+
+    <j:if test="${cachedContainer != null}">
+
+        <h2>Details</h2>
+
+        <table class="pane sortable" id="files">
+            <tr>
+                <td class="pane-header">Filename</td>
+                <td class="pane-header" style="width:2em">LineNumber</td>
+                <td class="pane-header" style="width:2em">CppCheckId</td>
+                <td class="pane-header" style="width:2em">Severity</td>
+                <td class="pane-header" style="width:60%">Message</td>
+            </tr>
+            <tbody>
+
+                <j:forEach var="elt" items="${cachedContainer.internalMap.values()}">
+                    <j:set var="cppcheckfile" value="${elt.cppcheckFile}"/>
+                    <tr>
+                        <td class="pane">
+                            <j:if test="${elt.isSourceIgnored()}">
+                                ${cppcheckfile.fileName}
+                            </j:if>
+                            <j:if test="${not elt.isSourceIgnored()}">
+                                <a href="source.${cppcheckfile.key}">${cppcheckfile.fileName}</a>
+                            </j:if>
+                        </td>
+                        <td class="pane">
+                            <j:if test="${elt.isSourceIgnored()}">
+                                ${cppcheckfile.lineNumber}
+                            </j:if>
+                            <j:if test="${not elt.isSourceIgnored()}">
+                                <a href="source.${cppcheckfile.key}#${cppcheckfile.lineNumber}">${cppcheckfile.lineNumber}
+                                </a>
+                            </j:if>
+                        </td>
+                        <g:format value="${cppcheckfile.cppCheckId}" severity="${cppcheckfile.severity}"/>
+                        <g:format value="${cppcheckfile.severity}" severity="${cppcheckfile.severity}"/>
+                        <g:format value="${cppcheckfile.message}" severity="${cppcheckfile.severity}"/>
+                    </tr>
+                </j:forEach>
+
+            </tbody>
+        </table>
+
+    </j:if>
+
 </j:jelly>