Fixed the lock contention problem on Queue.getItems()

From what I was told by Emanuele today in the office hours, this came up during the Copenhagen hackathon. (cherry picked from commit f1aa7ba) Conflicts: changelog.html
jenkinsci · Jan 24, 2013 · fd3d7b9 · fd3d7b9
1 parent 580e7de
commit fd3d7b9
Show file tree

Hide file tree

Showing 9 changed files with 90 additions and 12 deletions.
diff --git a/changelog.html b/changelog.html
@@ -58,6 +58,8 @@
   <li class=bug>
     Fix combobox ui component
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-16069">issue 16069</a>)
+  <li class=rfe>
+    Fixed the lock contention problem on <tt>Queue.getItems()</tt>
 </ul>
 </div><!--=TRUNK-END=-->
 

diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java
@@ -24,8 +24,10 @@
  */
 package hudson.model;
 
+import com.google.common.collect.ImmutableList;
 import hudson.AbortException;
 import hudson.BulkChange;
+import hudson.CopyOnWrite;
 import hudson.ExtensionList;
 import hudson.ExtensionPoint;
 import hudson.Util;
@@ -87,6 +89,7 @@
 import java.util.Map.Entry;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicLong;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -161,6 +164,41 @@ public class Queue extends ResourceController implements Saveable {
      */
     private final ItemList<BuildableItem> pendings = new ItemList<BuildableItem>();
 
+    private final CachedItemList itemsView = new CachedItemList();
+
+    /**
+     * Maintains a copy of {@link Queue#getItems()}
+     *
+     * @see Queue#getApproximateItemsQuickly()
+     */
+    private class CachedItemList {
+        /**
+         * The current cached value.
+         */
+        @CopyOnWrite
+        private volatile List<Item> itemsView = Collections.emptyList();
+        /**
+         * When does the cache info expire?
+         */
+        private final AtomicLong expires = new AtomicLong();
+
+        List<Item> get() {
+            long t = System.currentTimeMillis();
+            long d = expires.get();
+            if (t>d) {// need to refresh the cache
+                long next = t+1000;
+                if (expires.compareAndSet(d,next)) {
+                    // avoid concurrent cache update via CAS.
+                    // if the getItems() lock is contended,
+                    // some threads will end up serving stale data,
+                    // but that's OK.
+                    itemsView = ImmutableList.copyOf(getItems());
+                }
+            }
+            return itemsView;
+        }
+    }
+
     /**
      * Data structure created for each idle {@link Executor}.
      * This is a job offer from the queue to an executor.
@@ -609,6 +647,27 @@ public synchronized Item[] getItems() {
             r[idx++] = p;
         return r;
     }
+
+    /**
+     * Like {@link #getItems()}, but returns an approximation that might not be completely up-to-date.
+     *
+     * <p>
+     * At the expense of accuracy, this method does not lock {@link Queue} and therefore is faster
+     * in a highly concurrent situation.
+     *
+     * <p>
+     * The list obtained is an accurate snapshot of the queue at some point in the past. The snapshot
+     * is updated and normally no less than one second old, but this is a soft commitment that might
+     * get violated when the lock on {@link Queue} is highly contended.
+     *
+     * <p>
+     * This method is primarily added to make UI threads run faster.
+     *
+     * @since 1.483
+     */
+    public List<Item> getApproximateItemsQuickly() {
+        return itemsView.get();
+    }
 
     public synchronized Item getItem(int id) {
     	for (Item item: waitingList) if (item.id == id) return item;

diff --git a/core/src/main/java/hudson/model/View.java b/core/src/main/java/hudson/model/View.java
@@ -433,14 +433,14 @@ private boolean isDisjoint(Collection c1, Collection c2) {
         return true;
     }
 
-    public List<Queue.Item> getQueueItems() {
+    private List<Queue.Item> filterQueue(List<Queue.Item> base) {
         if (!isFilterQueue()) {
-            return Arrays.asList(Jenkins.getInstance().getQueue().getItems());
+            return base;
         }
 
         Collection<TopLevelItem> items = getItems();
         List<Queue.Item> result = new ArrayList<Queue.Item>();
-        for (Queue.Item qi : Jenkins.getInstance().getQueue().getItems()) {
+        for (Queue.Item qi : base) {
             if (items.contains(qi.task)) {
                 result.add(qi);
             } else
@@ -454,6 +454,14 @@ public List<Queue.Item> getQueueItems() {
         return result;
     }
 
+    public List<Queue.Item> getQueueItems() {
+        return filterQueue(Arrays.asList(Jenkins.getInstance().getQueue().getItems()));
+    }
+
+    public List<Queue.Item> getApproximateQueueItemsQuickly() {
+        return filterQueue(Jenkins.getInstance().getQueue().getApproximateItemsQuickly());
+    }
+
     /**
      * Returns the path relative to the context root.
      *

diff --git a/core/src/main/resources/hudson/model/ComputerSet/sidepanel.jelly b/core/src/main/resources/hudson/model/ComputerSet/sidepanel.jelly
@@ -36,7 +36,7 @@ THE SOFTWARE.
       <l:task icon="images/24x24/new-computer.png" href="new" title="${%New Node}" permission="${createPermission}" />
       <l:task icon="images/24x24/setting.png" href="configure" title="${%Configure}" permission="${app.ADMINISTER}" />
     </l:tasks>
-    <t:queue items="${app.queue.items}" />
+    <t:queue items="${app.queue.approximateItemsQuickly}" />
     <t:executors />
   </l:side-panel>
 </j:jelly>
diff --git a/core/src/main/resources/hudson/model/View/ajaxBuildQueue.jelly b/core/src/main/resources/hudson/model/View/ajaxBuildQueue.jelly
@@ -28,6 +28,6 @@ THE SOFTWARE.
 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:s="/lib/form">
   <l:ajax>
-    <t:queue items="${it.queueItems}" />
+    <t:queue items="${it.approximateItemsQuickly}" />
   </l:ajax>
 </j:jelly>
diff --git a/core/src/main/resources/hudson/model/View/sidepanel.jelly b/core/src/main/resources/hudson/model/View/sidepanel.jelly
@@ -70,7 +70,7 @@ THE SOFTWARE.
       <st:include page="tasks-bottom.jelly" it="${it.owner}" optional="true" />
       <t:actions />
     </l:tasks>
-    <t:queue items="${it.queueItems}" />
+    <t:queue items="${it.approximateQueueItemsQuickly}" />
     <t:executors computers="${it.computers}" />
     <j:forEach var="w" items="${it.widgets}">
       <st:include it="${w}" page="index.jelly" />

diff --git a/maven-plugin/src/main/java/hudson/maven/MavenModuleSet.java b/maven-plugin/src/main/java/hudson/maven/MavenModuleSet.java
@@ -29,7 +29,6 @@
 import hudson.CopyOnWrite;
 import hudson.EnvVars;
 import hudson.Extension;
-import hudson.ExtensionList;
 import hudson.FilePath;
 import hudson.Functions;
 import hudson.Indenter;
@@ -60,7 +59,6 @@
 import hudson.search.CollectionSearchIndex;
 import hudson.search.SearchIndexBuilder;
 import hudson.tasks.BuildStep;
-import hudson.tasks.BuildStepDescriptor;
 import hudson.tasks.BuildWrapper;
 import hudson.tasks.BuildWrappers;
 import hudson.tasks.Builder;
@@ -993,8 +991,19 @@ public void setMaven(String mavenName) {
      * Returns the {@link MavenModule}s that are in the queue.
      */
     public List<Queue.Item> getQueueItems() {
-        List<Queue.Item> r = new ArrayList<hudson.model.Queue.Item>();
-        for( Queue.Item item : Jenkins.getInstance().getQueue().getItems() ) {
+        return filter(Arrays.asList(Jenkins.getInstance().getQueue().getItems()));
+    }
+
+    /**
+     * Returns the {@link MavenModule}s that are in the queue.
+     */
+    public List<Queue.Item> getApproximateQueueItemsQuickly() {
+        return filter(Jenkins.getInstance().getQueue().getApproximateItemsQuickly());
+    }
+
+    private List<Queue.Item> filter(Collection<Queue.Item> base) {
+        List<Queue.Item> r = new ArrayList<Queue.Item>();
+        for( Queue.Item item : base) {
             Task t = item.task;
             if((t instanceof MavenModule && ((MavenModule)t).getParent()==this) || t ==this)
                 r.add(item);

diff --git a/maven-plugin/src/main/resources/hudson/maven/MavenModuleSet/ajaxBuildQueue.jelly b/maven-plugin/src/main/resources/hudson/maven/MavenModuleSet/ajaxBuildQueue.jelly
@@ -28,6 +28,6 @@ THE SOFTWARE.
 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:s="/lib/form">
   <l:ajax>
-    <t:queue items="${it.queueItems}" />
+    <t:queue items="${it.approximateQueueItemsQuickly}" />
   </l:ajax>
 </j:jelly>
diff --git a/maven-plugin/src/main/resources/hudson/maven/MavenModuleSet/sidepanel2.jelly b/maven-plugin/src/main/resources/hudson/maven/MavenModuleSet/sidepanel2.jelly
@@ -31,6 +31,6 @@ THE SOFTWARE.
       sense when modules are built in parallel.
       See related HUDSON-1892 for how this confuses users.
     -->
-    <t:queue items="${it.queueItems}"/>
+    <t:queue items="${it.approximateQueueItemsQuickly}"/>
   </j:if>
 </j:jelly>