Optimizations & simplifications pertaining to lazy-loading (#11320)

jglick · web-flow · commit bdf22a2dfd89 · 2025-11-23T04:36:09.000-07:00
* Avoid `TransientActionFactory` from `Job.getPermalinks`

* Optimizing `PeepholePermalink.RunListener` to avoid resolving old targets

* Simplified `RunMixIn` handling of next/previous build
diff --git a/core/src/main/java/hudson/model/Job.java b/core/src/main/java/hudson/model/Job.java
@@ -1119,12 +1119,15 @@ public long getEstimatedDuration() {
      *
      * @return never null
      */
+    @SuppressWarnings("deprecation")
     public PermalinkList getPermalinks() {
         PeepholePermalink.initialized();
         // TODO: shall we cache this?
         PermalinkList permalinks = new PermalinkList(Permalink.BUILTIN);
-        for (PermalinkProjectAction ppa : getActions(PermalinkProjectAction.class)) {
-            permalinks.addAll(ppa.getPermalinks());
+        for (var action : getActions()) {
+            if (action instanceof PermalinkProjectAction ppa) {
+                permalinks.addAll(ppa.getPermalinks());
+            }
         }
         return permalinks;
     }
diff --git a/core/src/main/java/hudson/model/Run.java b/core/src/main/java/hudson/model/Run.java
@@ -191,23 +191,6 @@ public abstract class Run<JobT extends Job<JobT, RunT>, RunT extends Run<JobT, R
      */
     private long queueId = Run.QUEUE_ID_UNKNOWN;
 
-    /**
-     * Previous build. Can be null.
-     * TODO JENKINS-22052 this is not actually implemented any more
-     *
-     * External code should use {@link #getPreviousBuild()}
-     */
-    @Restricted(NoExternalUse.class)
-    protected transient volatile RunT previousBuild;
-
-    /**
-     * Next build. Can be null.
-     *
-     * External code should use {@link #getNextBuild()}
-     */
-    @Restricted(NoExternalUse.class)
-    protected transient volatile RunT nextBuild;
-
     /**
      * Pointer to the next younger build in progress. This data structure is lazily updated,
      * so it may point to the build that's already completed. This pointer is set to 'this'
@@ -801,23 +784,19 @@ public int getNumber() {
     }
 
     /**
-     * Called by {@link RunMap} to drop bi-directional links in preparation for
-     * deleting a build.
+     * Called by {@link RunMap} in preparation for deleting a build.
      * @see jenkins.model.lazy.LazyBuildMixIn.RunMixIn#dropLinks
      * @since 1.556
      */
     protected void dropLinks() {
-        if (nextBuild != null)
-            nextBuild.previousBuild = previousBuild;
-        if (previousBuild != null)
-            previousBuild.nextBuild = nextBuild;
     }
 
     /**
      * @see jenkins.model.lazy.LazyBuildMixIn.RunMixIn#getPreviousBuild
      */
     public @CheckForNull RunT getPreviousBuild() {
-        return previousBuild;
+        // TODO could be implemented for benefit of ExternalRun
+        return null;
     }
 
     /**
@@ -960,7 +939,8 @@ protected void dropLinks() {
      * @see jenkins.model.lazy.LazyBuildMixIn.RunMixIn#getNextBuild
      */
     public @CheckForNull RunT getNextBuild() {
-        return nextBuild;
+        // TODO could be implemented for benefit of ExternalRun
+        return null;
     }
 
 
diff --git a/core/src/main/java/jenkins/model/PeepholePermalink.java b/core/src/main/java/jenkins/model/PeepholePermalink.java
@@ -90,7 +90,24 @@ protected File getPermalinkFile(Job<?, ?> job) {
      */
     @Override
     public Run<?, ?> resolve(Job<?, ?> job) {
-        return ExtensionList.lookupFirst(Cache.class).get(job, getId()).resolve(this, job, getId());
+        return get(job).resolve(this, job, getId());
+    }
+
+    Cache.PermalinkTarget get(Job<?, ?> job) {
+        return ExtensionList.lookupFirst(Cache.class).get(job, getId());
+    }
+
+    int resolveNumber(Job<?, ?> job) {
+        // TODO Java 21+ use patterns
+        var pt = get(job);
+        if (pt instanceof Cache.Some some) {
+            return some.number;
+        } else if (pt instanceof Cache.None) {
+            return 0;
+        } else { // Unknown
+            var b = pt.resolve(this, job, getId());
+            return b != null ? b.number : 0;
+        }
     }
 
     /**
@@ -312,7 +329,7 @@ public static class RunListenerImpl extends RunListener<Run<?, ?>> {
         public void onDeleted(Run run) {
             Job<?, ?> j = run.getParent();
             for (PeepholePermalink pp : Util.filter(j.getPermalinks(), PeepholePermalink.class)) {
-                if (pp.resolve(j) == run) {
+                if (pp.resolveNumber(j) == run.number) {
                     Run<?, ?> r = pp.find(run.getPreviousBuild());
                     LOGGER.fine(() -> "Updating " + pp.getId() + " permalink from deleted " + run + " to " + (r == null ? -1 : r.getNumber()));
                     pp.updateCache(j, r);
@@ -328,8 +345,7 @@ public void onCompleted(Run<?, ?> run, @NonNull TaskListener listener) {
             Job<?, ?> j = run.getParent();
             for (PeepholePermalink pp : Util.filter(j.getPermalinks(), PeepholePermalink.class)) {
                 if (pp.apply(run)) {
-                    Run<?, ?> cur = pp.resolve(j);
-                    if (cur == null || cur.getNumber() < run.getNumber()) {
+                    if (pp.resolveNumber(j) < run.getNumber()) {
                         LOGGER.fine(() -> "Updating " + pp.getId() + " permalink to completed " + run);
                         pp.updateCache(j, run);
                     }
diff --git a/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java b/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java
@@ -24,7 +24,6 @@
 
 package jenkins.model.lazy;
 
-import static java.util.logging.Level.FINER;
 
 import edu.umd.cs.findbugs.annotations.CheckForNull;
 import edu.umd.cs.findbugs.annotations.NonNull;
@@ -46,7 +45,6 @@
 import java.nio.file.Files;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Objects;
 import java.util.TreeMap;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -194,7 +192,6 @@ public final synchronized RunT newBuild() throws IOException {
                return newBuild();
             }
             builds.put(lastBuild);
-            lastBuild.getPreviousBuild(); // JENKINS-20662: create connection to previous build
             return lastBuild;
         } catch (InvocationTargetException e) {
             LOGGER.log(Level.WARNING, String.format("A new build could not be created in job %s", asJob().getFullName()), e);
@@ -334,32 +331,6 @@ public interface LazyLoadingRun<JobT extends Job<JobT, RunT> & Queue.Task & Lazy
      */
     public abstract static class RunMixIn<JobT extends Job<JobT, RunT> & Queue.Task & LazyBuildMixIn.LazyLoadingJob<JobT, RunT>, RunT extends Run<JobT, RunT> & LazyLoadingRun<JobT, RunT>> {
 
-        /**
-         * Pointers to form bi-directional link between adjacent runs using
-         * {@link LazyBuildMixIn}.
-         *
-         * <p>
-         * Some {@link Run}s do lazy-loading, so we don't use
-         * {@link #previousBuildR} and {@link #nextBuildR}, and instead use these
-         * fields and point to {@link #selfReference} (or {@link #none}) of
-         * adjacent builds.
-         */
-        private volatile BuildReference<RunT> previousBuildR, nextBuildR;
-
-        /**
-         * Used in {@link #previousBuildR} and {@link #nextBuildR} to indicate
-         * that we know there is no next/previous build (as opposed to {@code null},
-         * which is used to indicate we haven't determined if there is a next/previous
-         * build.)
-         */
-        @SuppressWarnings({"unchecked", "rawtypes"})
-        private static final BuildReference NONE = new BuildReference("NONE", null);
-
-        @SuppressWarnings("unchecked")
-        private BuildReference<RunT> none() {
-            return NONE;
-        }
-
         private BuildReference<RunT> selfReference;
 
         protected RunMixIn() {}
@@ -380,19 +351,6 @@ public final synchronized BuildReference<RunT> createReference() {
          * To implement {@link Run#dropLinks}.
          */
         public final void dropLinks() {
-            if (nextBuildR != null) {
-                RunT nb = nextBuildR.get();
-                if (nb != null) {
-                    nb.getRunMixIn().previousBuildR = previousBuildR;
-                }
-            }
-            if (previousBuildR != null) {
-                RunT pb = previousBuildR.get();
-                if (pb != null) {
-                    pb.getRunMixIn().nextBuildR = nextBuildR;
-                }
-            }
-
             // make this build object unreachable by other Runs
             createReference().clear();
         }
@@ -401,69 +359,14 @@ public final void dropLinks() {
          * To implement {@link Run#getPreviousBuild}.
          */
         public final RunT getPreviousBuild() {
-            while (true) {
-                BuildReference<RunT> r = previousBuildR;    // capture the value once
-
-                if (r == null) {
-                    // having two neighbors pointing to each other is important to make RunMap.removeValue work
-                    JobT _parent = Objects.requireNonNull(asRun().getParent(), "no parent for " + asRun().number);
-                    RunT pb = _parent.getLazyBuildMixIn()._getRuns().search(asRun().number - 1, AbstractLazyLoadRunMap.Direction.DESC);
-                    if (pb != null) {
-                        pb.getRunMixIn().nextBuildR = createReference();   // establish bi-di link
-                        this.previousBuildR = pb.getRunMixIn().createReference();
-                        LOGGER.log(FINER, "Linked {0}<->{1} in getPreviousBuild()", new Object[]{this, pb});
-                        return pb;
-                    } else {
-                        this.previousBuildR = none();
-                        return null;
-                    }
-                }
-                if (r == none()) {
-                    return null;
-                }
-
-                RunT referent = r.get();
-                if (referent != null) {
-                    return referent;
-                }
-
-                // the reference points to a GC-ed object, drop the reference and do it again
-                this.previousBuildR = null;
-            }
+            return asRun().getParent().getLazyBuildMixIn()._getRuns().search(asRun().number - 1, AbstractLazyLoadRunMap.Direction.DESC);
         }
 
         /**
          * To implement {@link Run#getNextBuild}.
          */
         public final RunT getNextBuild() {
-            while (true) {
-                BuildReference<RunT> r = nextBuildR;    // capture the value once
-
-                if (r == null) {
-                    // having two neighbors pointing to each other is important to make RunMap.removeValue work
-                    RunT nb = asRun().getParent().getLazyBuildMixIn()._getRuns().search(asRun().number + 1, AbstractLazyLoadRunMap.Direction.ASC);
-                    if (nb != null) {
-                        nb.getRunMixIn().previousBuildR = createReference();   // establish bi-di link
-                        this.nextBuildR = nb.getRunMixIn().createReference();
-                        LOGGER.log(FINER, "Linked {0}<->{1} in getNextBuild()", new Object[]{this, nb});
-                        return nb;
-                    } else {
-                        this.nextBuildR = none();
-                        return null;
-                    }
-                }
-                if (r == none()) {
-                    return null;
-                }
-
-                RunT referent = r.get();
-                if (referent != null) {
-                    return referent;
-                }
-
-                // the reference points to a GC-ed object, drop the reference and do it again
-                this.nextBuildR = null;
-            }
+            return asRun().getParent().getLazyBuildMixIn()._getRuns().search(asRun().number + 1, AbstractLazyLoadRunMap.Direction.ASC);
         }
 
     }

Original file line number	Diff line number	Diff line change
`@@ -1119,12 +1119,15 @@ public long getEstimatedDuration() {`
`1119`	`1119`	`*`
`1120`	`1120`	`* @return never null`
`1121`	`1121`	`*/`
	`1122`	`+ @SuppressWarnings("deprecation")`
`1122`	`1123`	`public PermalinkList getPermalinks() {`
`1123`	`1124`	`PeepholePermalink.initialized();`
`1124`	`1125`	`// TODO: shall we cache this?`
`1125`	`1126`	`PermalinkList permalinks = new PermalinkList(Permalink.BUILTIN);`
`1126`		`- for (PermalinkProjectAction ppa : getActions(PermalinkProjectAction.class)) {`
`1127`		`- permalinks.addAll(ppa.getPermalinks());`
	`1127`	`+ for (var action : getActions()) {`
	`1128`	`+ if (action instanceof PermalinkProjectAction ppa) {`
	`1129`	`+ permalinks.addAll(ppa.getPermalinks());`
	`1130`	`+ }`
`1128`	`1131`	`}`
`1129`	`1132`	`return permalinks;`
`1130`	`1133`	`}`