[JENKINS-76075]: Optimise BuildReferenceMapAdapter and LogRotator: defer reference resolution (#11038)

Co-authored-by: Dmytro Ukhlov <dukhlov@csod.com>

core/src/main/java/hudson/tasks/LogRotator.java

@@ -32,7 +32,6 @@ import edu.umd.cs.findbugs.annotations.NonNull;
 import hudson.Extension;
 import hudson.model.Job;
 import hudson.model.Run;
-import hudson.util.RunList;
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.Calendar;
@@ -165,20 +164,12 @@ public class LogRotator extends BuildDiscarder {
         Run lstb = removeLastBuild ? null : job.getLastStableBuild();
 
         if (numToKeep != -1) {
-            // Note that RunList.size is deprecated, and indeed here we are loading all the builds of the job.
+            job.getBuildsAsMap().entrySet().stream().skip(numToKeep).map(Map.Entry::getValue)
-            // However we would need to load the first numToKeep anyway, just to skip over them;
+                    .filter(r -> !shouldKeepRun(r, lsb, lstb)).forEach(r -> {
-            // and we would need to load the rest anyway, to delete them.
-            // (Using RunMap.headMap would not suffice, since we do not know if some recent builds have been deleted for other reasons,
-            // so simply subtracting numToKeep from the currently last build number might cause us to delete too many.)
-            RunList<? extends Run<?, ?>> builds = job.getBuilds();
-            for (Run r : builds.subList(Math.min(builds.size(), numToKeep), builds.size())) {
-                if (shouldKeepRun(r, lsb, lstb)) {
-                    continue;
-                }
                 LOGGER.log(FINE, "{0} is to be removed", r);
                 try { r.delete(); }
                 catch (IOException ex) { exceptionMap.computeIfAbsent(r, key -> new HashSet<>()).add(ex); }
-            }
+            });
         }
 
         if (daysToKeep != -1) {
@@ -199,15 +190,12 @@ public class LogRotator extends BuildDiscarder {
         }
 
         if (artifactNumToKeep != null && artifactNumToKeep != -1) {
-            RunList<? extends Run<?, ?>> builds = job.getBuilds();
+            job.getBuildsAsMap().entrySet().stream().skip(artifactNumToKeep).map(Map.Entry::getValue)
-            for (Run r : builds.subList(Math.min(builds.size(), artifactNumToKeep), builds.size())) {
+                    .filter(r -> !shouldKeepRun(r, lsb, lstb)).forEach(r -> {
-                if (shouldKeepRun(r, lsb, lstb)) {
-                    continue;
-                }
                 LOGGER.log(FINE, "{0} is to be purged of artifacts", r);
                 try { r.deleteArtifacts(); }
                 catch (IOException ex) { exceptionMap.computeIfAbsent(r, key -> new HashSet<>()).add(ex); }
-            }
+            });
         }
 
         if (artifactDaysToKeep != null && artifactDaysToKeep != -1) {

core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java

@@ -82,11 +82,6 @@ import org.kohsuke.accmod.restrictions.NoExternalUse;
  * from concurrent modifications, where another thread deletes a build while one thread iterates them.
  *
  * <p>
- * Some of the {@link SortedMap} operations are inefficiently implemented, by
- * loading all the build records eagerly. We hope to replace
- * these implementations by more efficient lazy-loading ones as we go.
- *
- * <p>
  * Object lock of {@code this} is used to make sure mutation occurs sequentially.
  * That is, ensure that only one thread is actually calling {@link #retrieve(File)} and
  * updating {@link jenkins.model.lazy.AbstractLazyLoadRunMap#core}.
@@ -116,7 +111,7 @@ public abstract class AbstractLazyLoadRunMap<R> extends AbstractMap<Integer, R>
     }
 
     @Override
-    public Set<Entry<Integer, R>> entrySet() {
+    public Set<Map.Entry<Integer, R>> entrySet() {
         assert baseDirInitialized();
         return adapter.entrySet();
     }

core/src/main/java/jenkins/model/lazy/BuildReference.java

@@ -71,7 +71,7 @@ public final class BuildReference<R> {
 
     /**
      * check if reference holder set.
-     * means there war a try to load build object and we have some result of that try
+     * means there was a try to load build object and we have some result of that try
      *
      * @return true if there was a try to
      */

core/src/main/java/jenkins/model/lazy/BuildReferenceMapAdapter.java

@@ -97,7 +97,7 @@ class BuildReferenceMapAdapter<R> extends AbstractMap<Integer, R> implements Sor
     }
 
     @Override
-    public Set<Entry<Integer, R>> entrySet() {
+    public Set<Map.Entry<Integer, R>> entrySet() {
         return entrySet;
     }
 
@@ -171,7 +171,7 @@ class BuildReferenceMapAdapter<R> extends AbstractMap<Integer, R> implements Sor
         public Iterator<Integer> iterator() {
             return new AdaptedIterator<>(BuildReferenceMapAdapter.this.entrySet().iterator()) {
                 @Override
-                protected Integer adapt(Entry<Integer, R> e) {
+                protected Integer adapt(Map.Entry<Integer, R> e) {
                     return e.getKey();
                 }
             };
@@ -227,7 +227,7 @@ class BuildReferenceMapAdapter<R> extends AbstractMap<Integer, R> implements Sor
         public Iterator<R> iterator() {
             return new AdaptedIterator<>(BuildReferenceMapAdapter.this.entrySet().iterator()) {
                 @Override
-                protected R adapt(Entry<Integer, R> e) {
+                protected R adapt(Map.Entry<Integer, R> e) {
                     return e.getValue();
                 }
             };
@@ -239,7 +239,7 @@ class BuildReferenceMapAdapter<R> extends AbstractMap<Integer, R> implements Sor
         }
     }
 
-    private class EntrySetAdapter extends AbstractSet<Entry<Integer, R>> {
+    private class EntrySetAdapter extends AbstractSet<Map.Entry<Integer, R>> {
         @Override
         public int size() {
             return BuildReferenceMapAdapter.this.core.size();
@@ -268,13 +268,24 @@ class BuildReferenceMapAdapter<R> extends AbstractMap<Integer, R> implements Sor
         }
 
         @Override
-        public Iterator<Entry<Integer, R>> iterator() {
+        public Iterator<Map.Entry<Integer, R>> iterator() {
             return new Iterator<>() {
-                private Entry<Integer, R> current;
+                private Map.Entry<Integer, R> current;
-                private final Iterator<Entry<Integer, R>> it = Iterators.removeNull(Iterators.map(
+                private final Iterator<Map.Entry<Integer, R>> it = Iterators.removeNull(Iterators.map(
                         BuildReferenceMapAdapter.this.core.entrySet().iterator(), coreEntry -> {
-                            R v = BuildReferenceMapAdapter.this.resolver.resolveBuildRef(coreEntry.getValue());
+                            BuildReference<R> ref = coreEntry.getValue();
-                            return v == null ? null : new AbstractMap.SimpleEntry<>(coreEntry.getKey(), v);
+                            if (!ref.isSet()) {
+                                R r = resolver.resolveBuildRef(ref);
+                                // load not loaded or unloadable build
+                                if (r == null) {
+                                    return null;
+                                }
+                                return new EntryAdapter(coreEntry, r);
+                            }
+                            if (ref.isUnloadable()) {
+                                return null;
+                            }
+                            return new EntryAdapter(coreEntry);
                         }));
 
                 @Override
@@ -283,7 +294,7 @@ class BuildReferenceMapAdapter<R> extends AbstractMap<Integer, R> implements Sor
                 }
 
                 @Override
-                public Entry<Integer, R> next() {
+                public Map.Entry<Integer, R> next() {
                     return current = it.next();
                 }
 
@@ -303,6 +314,59 @@ class BuildReferenceMapAdapter<R> extends AbstractMap<Integer, R> implements Sor
         }
     }
 
+    private class EntryAdapter implements Entry<Integer, R> {
+        private final Map.Entry<Integer, BuildReference<R>> coreEntry;
+        private volatile R resolvedValue;
+
+        EntryAdapter(Map.Entry<Integer, BuildReference<R>> coreEntry) {
+            this(coreEntry, null);
+        }
+
+        EntryAdapter(Map.Entry<Integer, BuildReference<R>> coreEntry, R resolvedValue) {
+            this.coreEntry = coreEntry;
+            this.resolvedValue = resolvedValue;
+        }
+
+        private Map.Entry<Integer, R> getResolvedEntry() {
+            return new AbstractMap.SimpleEntry<>(getKey(), getValue());
+        }
+
+        @Override
+        public Integer getKey() {
+            return coreEntry.getKey();
+        }
+
+        @Override
+        public R getValue() {
+            R value = resolvedValue;
+            if (value != null) {
+                return value;
+            }
+            return resolvedValue = resolver.resolveBuildRef(coreEntry.getValue());
+        }
+
+        @Override
+        public R setValue(R value) {
+            // BuildReferenceAdapter is read only
+            throw new UnsupportedOperationException();
+        }
+
+        @Override
+        public String toString() {
+            return getResolvedEntry().toString();
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            return (o instanceof Map.Entry<?, ?>) && getResolvedEntry().equals(o);
+        }
+
+        @Override
+        public int hashCode() {
+            return getResolvedEntry().hashCode();
+        }
+    }
+
     /**
      * An interface for resolving build references into actual build instances
      * and extracting basic metadata from them.