[FIXED JENKINS-33467] Do not store redundant copies of Cause in Cause…

…Action. (cherry picked from commit 6ae54ad)
jenkinsci · Apr 25, 2016 · 916e759 · daniel-beck · May 9, 2016 · aheritier
1 parent 35cbc5c
commit 916e759
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 28 deletions.
diff --git a/core/src/main/java/hudson/model/CauseAction.java b/core/src/main/java/hudson/model/CauseAction.java
@@ -34,6 +34,7 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -47,35 +48,55 @@ public class CauseAction implements FoldableAction, RunAction2 {
     @Deprecated
     // there can be multiple causes, so this is deprecated
     private transient Cause cause;
-
-    private List<Cause> causes = new ArrayList<Cause>();
+
+    /** @deprecated JENKINS-33467 inefficient */
+    @Deprecated
+    private transient List<Cause> causes;
+
+    private Map<Cause,Integer> causeBag = new LinkedHashMap<>();
 
     public CauseAction(Cause c) {
-   		this.causes.add(c);
+   		this.causeBag.put(c, 1);
    	}
 
+    private void addCause(Cause c) {
+        synchronized (causeBag) {
+            Integer cnt = causeBag.get(c);
+            causeBag.put(c, cnt == null ? 1 : cnt + 1);
+        }
+    }
+    private void addCauses(Collection<? extends Cause> causes) {
+        for (Cause cause : causes) {
+            addCause(cause);
+        }
+    }
+
     public CauseAction(Cause... c) {
    		this(Arrays.asList(c));
    	}
 
     public CauseAction(Collection<? extends Cause> causes) {
-   		this.causes.addAll(causes);
+   		addCauses(causes);
    	}
 
    	public CauseAction(CauseAction ca) {
-   		this.causes.addAll(ca.causes);
+   		addCauses(ca.getCauses());
    	}
 
 	@Exported(visibility=2)
 	public List<Cause> getCauses() {
-		return causes;
+		List<Cause> r = new ArrayList<>();
+        for (Map.Entry<Cause,Integer> entry : causeBag.entrySet()) {
+            r.addAll(Collections.nCopies(entry.getValue(), entry.getKey()));
+        }
+        return r;
 	}
 
     /**
      * Finds the cause of the specific type.
      */
     public <T extends Cause> T findCause(Class<T> type) {
-        for (Cause c : causes)
+        for (Cause c : causeBag.keySet())
             if (type.isInstance(c))
                 return type.cast(c);
         return null;
@@ -99,14 +120,7 @@ public String getUrlName() {
      * @return Map of Cause to number of occurrences of that Cause
      */
     public Map<Cause,Integer> getCauseCounts() {
-        Map<Cause,Integer> result = new LinkedHashMap<Cause,Integer>();
-        for (Cause c : causes) {
-            if (c != null) {
-                Integer i = result.get(c);
-                result.put(c, i == null ? 1 : i.intValue() + 1);
-            }
-        }
-        return result;
+        return Collections.unmodifiableMap(causeBag);
     }
 
     /**
@@ -115,12 +129,14 @@ public Map<Cause,Integer> getCauseCounts() {
      */
     @Deprecated
     public String getShortDescription() {
-        if(causes.isEmpty())    return "N/A";
-        return causes.get(0).getShortDescription();
+        if (causeBag.isEmpty()) {
+            return "N/A";
+        }
+        return causeBag.keySet().iterator().next().getShortDescription();
     }
 
     @Override public void onLoad(Run<?,?> owner) {
-        for (Cause c : causes) {
+        for (Cause c : causeBag.keySet()) {
             if (c != null) {
                 c.onLoad(owner);
             }
@@ -131,7 +147,7 @@ public String getShortDescription() {
      * When hooked up to build, notify {@link Cause}s.
      */
     @Override public void onAttached(Run<?,?> owner) {
-        for (Cause c : causes) {
+        for (Cause c : causeBag.keySet()) {
             if (c != null) {
                 c.onAddedTo(owner);
             }
@@ -141,7 +157,7 @@ public String getShortDescription() {
     public void foldIntoExisting(hudson.model.Queue.Item item, Task owner, List<Action> otherActions) {
         CauseAction existing = item.getAction(CauseAction.class);
         if (existing!=null) {
-            existing.causes.addAll(this.causes);
+            existing.addCauses(getCauses());
             return;
         }
         // no CauseAction found, so add a copy of this one
@@ -153,9 +169,19 @@ public static class ConverterImpl extends XStream2.PassthruConverter<CauseAction
         @Override protected void callback(CauseAction ca, UnmarshallingContext context) {
             // if we are being read in from an older version
             if (ca.cause != null) {
-                if (ca.causes == null) ca.causes = new ArrayList<Cause>();
-                ca.causes.add(ca.cause);
+                if (ca.causeBag == null) {
+                    ca.causeBag = new LinkedHashMap<>();
+                }
+                ca.addCause(ca.cause);
                 OldDataMonitor.report(context, "1.288");
+                ca.cause = null;
+            } else if (ca.causes != null) {
+                if (ca.causeBag == null) {
+                    ca.causeBag = new LinkedHashMap<>();
+                }
+                ca.addCauses(ca.causes);
+                OldDataMonitor.report(context, "1.653");
+                ca.causes = null;
             }
         }
     }

diff --git a/test/src/test/java/hudson/model/QueueTest.java b/test/src/test/java/hudson/model/QueueTest.java
@@ -305,6 +305,7 @@ public static final class FileItemPersistenceTestServlet extends HttpServlet {
         }
     }
 
+    @Issue("JENKINS-33467")
     @Test public void foldableCauseAction() throws Exception {
         final OneShotEvent buildStarted = new OneShotEvent();
         final OneShotEvent buildShouldComplete = new OneShotEvent();
@@ -348,14 +349,13 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
         StringBuilder causes = new StringBuilder();
         for (Cause c : ca.getCauses()) causes.append(c.getShortDescription() + "\n");
         assertEquals("Build causes should have all items, even duplicates",
-                "Started by user SYSTEM\nStarted by an SCM change\n"
-                + "Started by user SYSTEM\nStarted by timer\n"
+                "Started by user SYSTEM\nStarted by user SYSTEM\n"
+                + "Started by an SCM change\nStarted by an SCM change\nStarted by an SCM change\n"
+                + "Started by timer\nStarted by timer\n"
                 + "Started by remote host 1.2.3.4 with note: test\n"
-                + "Started by remote host 4.3.2.1 with note: test\n"
-                + "Started by an SCM change\n"
                 + "Started by remote host 1.2.3.4 with note: test\n"
-                + "Started by remote host 1.2.3.4 with note: foo\n"
-                + "Started by an SCM change\nStarted by timer\n",
+                + "Started by remote host 4.3.2.1 with note: test\n"
+                + "Started by remote host 1.2.3.4 with note: foo\n",
                 causes.toString());
 
         // View for build should group duplicates
@@ -369,6 +369,7 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
                         + "Started by remote host 1.2.3.4 with note: test (2 times) "
                         + "Started by remote host 4.3.2.1 with note: test "
                         + "Started by remote host 1.2.3.4 with note: foo"));
+        System.out.println(new XmlFile(new File(build.getRootDir(), "build.xml")).asString());
     }
 
     @Issue("JENKINS-8790")