From 6859e0e47d5b2f164bccb3b48ba3017e500b2a18 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 10 Dec 2024 10:08:25 -0500 Subject: [PATCH] `PeepholePermalink.Cache` --- core/src/main/java/hudson/model/Job.java | 6 +- .../java/jenkins/model/PeepholePermalink.java | 279 ++++++++++++------ .../jenkins/model/PeepholePermalinkTest.java | 2 +- .../model/lazy/LazyBuildMixInTest.java | 37 +++ 4 files changed, 228 insertions(+), 96 deletions(-) diff --git a/core/src/main/java/hudson/model/Job.java b/core/src/main/java/hudson/model/Job.java index 7d140656e363..ee9a566906df 100644 --- a/core/src/main/java/hudson/model/Job.java +++ b/core/src/main/java/hudson/model/Job.java @@ -853,7 +853,7 @@ public RunT getBuildForCLI(@Argument(required = true, metaVar = "BUILD#", usage } /** - * Gets the youngest build #m that satisfies {@code n<=m}. + * Gets the oldest build #m that satisfies {@code m ≥ n}. * * This is useful when you'd like to fetch a build but the exact build might * be already gone (deleted, rotated, etc.) @@ -868,7 +868,7 @@ public RunT getNearestBuild(int n) { } /** - * Gets the latest build #m that satisfies {@code m<=n}. + * Gets the newest build #m that satisfies {@code m ≤ n}. * * This is useful when you'd like to fetch a build but the exact build might * be already gone (deleted, rotated, etc.) @@ -977,7 +977,7 @@ public File getBuildDir() { protected abstract void removeRun(RunT run); /** - * Returns the last build. + * Returns the newest build. * @see LazyBuildMixIn#getLastBuild */ @Exported diff --git a/core/src/main/java/jenkins/model/PeepholePermalink.java b/core/src/main/java/jenkins/model/PeepholePermalink.java index 62185f39f1ae..de6844acca55 100644 --- a/core/src/main/java/jenkins/model/PeepholePermalink.java +++ b/core/src/main/java/jenkins/model/PeepholePermalink.java @@ -4,6 +4,8 @@ import edu.umd.cs.findbugs.annotations.NonNull; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import hudson.Extension; +import hudson.ExtensionList; +import hudson.ExtensionPoint; import hudson.Util; import hudson.model.Job; import hudson.model.PermalinkProjectAction.Permalink; @@ -14,6 +16,7 @@ import hudson.util.AtomicFileWriter; import java.io.File; import java.io.IOException; +import java.io.Serializable; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.util.HashMap; @@ -24,6 +27,7 @@ import java.util.logging.Logger; import java.util.stream.Stream; import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.Beta; import org.kohsuke.accmod.restrictions.NoExternalUse; /** @@ -63,14 +67,6 @@ */ public abstract class PeepholePermalink extends Permalink implements Predicate> { - /** - * JENKINS-22822: avoids rereading caches. - * Top map keys are {@code builds} directories. - * Inner maps are from permalink name to build number. - * Synchronization is first on the outer map, then on the inner. - */ - private static final Map> caches = new HashMap<>(); - /** * Checks if the given build satisfies the peep-hole criteria. * @@ -94,115 +90,216 @@ protected File getPermalinkFile(Job job) { */ @Override public Run resolve(Job job) { - Map cache = cacheFor(job.getBuildDir()); - int n; - synchronized (cache) { - n = cache.getOrDefault(getId(), 0); + return ExtensionList.lookupFirst(Cache.class).get(job, getId()).resolve(this, job, getId()); + } + + /** + * Start from the build 'b' and locate the build that matches the criteria going back in time + */ + @CheckForNull + private Run find(@CheckForNull Run b) { + while (b != null && !apply(b)) { + b = b.getPreviousBuild(); } - if (n == RESOLVES_TO_NONE) { - return null; + return b; + } + + /** + * Remembers the value 'n' in the cache for future {@link #resolve(Job)}. + */ + protected void updateCache(@NonNull Job job, @CheckForNull Run b) { + ExtensionList.lookupFirst(Cache.class).put(job, getId(), b != null ? new Cache.Some(b.getNumber()) : Cache.NONE); + } + + /** + * Persistable cache of peephole permalink targets. + */ + @Restricted(Beta.class) + public interface Cache extends ExtensionPoint { + + /** Cacheable target of a permalink. */ + sealed interface PermalinkTarget extends Serializable { + + /** + * Implementation of {@link #resolve(Job)}. + * This may update the cache if it was missing or found to be invalid. + */ + @Restricted(NoExternalUse.class) + @CheckForNull + Run resolve(@NonNull PeepholePermalink pp, @NonNull Job job, @NonNull String id); + + /** + * Partial implementation of {@link #resolve(PeepholePermalink, Job, String)} when searching. + * @param b if set, the newest build to even consider when searching + */ + @Restricted(NoExternalUse.class) + @CheckForNull + default Run search(@NonNull PeepholePermalink pp, @NonNull Job job, @NonNull String id, @CheckForNull Run b) { + if (b == null) { + // no cache + b = job.getLastBuild(); + } + // start from the build 'b' and locate the build that matches the criteria going back in time + b = pp.find(b); + pp.updateCache(job, b); + return b; + } + } - Run b; - if (n > 0) { - b = job.getBuildByNumber(n); - if (b != null && apply(b)) { - return b; // found it (in the most efficient way possible) + + /** + * The cache entry for this target is missing. + */ + record Unknown() implements PermalinkTarget { + @Override + public Run resolve(PeepholePermalink pp, Job job, String id) { + return search(pp, job, id, null); } - } else { - b = null; } - // the cache is stale. start the search - if (b == null) { - b = job.getNearestOldBuild(n); + Unknown UNKNOWN = new Unknown(); + + /** + * The cache entry for this target is present. + */ + sealed interface Known extends PermalinkTarget {} + + /** There is known to be no matching build. */ + record None() implements Known { + @Override + public Run resolve(PeepholePermalink pp, Job job, String id) { + return null; + } } - if (b == null) { - // no cache - b = job.getLastBuild(); + /** Singleton of {@link None}. */ + None NONE = new None(); + + /** A matching build, indicated by {@link Run#getNumber}. */ + record Some(int number) implements Known { + @Override + public Run resolve(PeepholePermalink pp, Job job, String id) { + Run b = job.getBuildByNumber(number); + if (b != null && pp.apply(b)) { + return b; // found it (in the most efficient way possible) + } + // the cache is stale. start the search + if (b == null) { + b = job.getNearestOldBuild(number); + } + return search(pp, job, id, b); + } } - // start from the build 'b' and locate the build that matches the criteria going back in time - b = find(b); + /** + * Looks for any existing cache hit. + * @param id {@link #getId} + * @return {@link Some} or {@link #NONE} or {@link #UNKNOWN} + */ + @NonNull PermalinkTarget get(@NonNull Job job, @NonNull String id); - updateCache(job, b); - return b; + /** + * Updates the cache. + * Note that this may be called not just when a build completes or is deleted + * (meaning that the logical value of the cache has changed), + * but also when {@link #resolve} has failed to find a cached value + * (or determined that a previously cached value is in fact invalid). + * @param id {@link #getId} + * @param target {@link Some} or {@link #NONE} + */ + void put(@NonNull Job job, @NonNull String id, @NonNull Known target); } /** - * Start from the build 'b' and locate the build that matches the criteria going back in time + * Default cache based on a {@code permalinks} file in the build directory. + * There is one line per cached permalink, in the format {@code lastStableBuild 123} + * or (for a negative cache) {@code lastFailedBuild -1}. */ - private Run find(Run b) { - //noinspection StatementWithEmptyBody - for ( ; b != null && !apply(b); b = b.getPreviousBuild()) - ; - return b; - } + @Restricted(NoExternalUse.class) + @Extension(ordinal = -1000) + public static final class DefaultCache implements Cache { + + /** + * JENKINS-22822: avoids rereading caches. + * Top map keys are {@code builds} directories. + * Inner maps are from permalink name to target. + * Synchronization is first on the outer map, then on the inner. + */ + private final Map> caches = new HashMap<>(); - private static @NonNull Map cacheFor(@NonNull File buildDir) { - synchronized (caches) { - Map cache = caches.get(buildDir); - if (cache == null) { - cache = load(buildDir); - caches.put(buildDir, cache); + @Override + public PermalinkTarget get(Job job, String id) { + var cache = cacheFor(job.getBuildDir()); + synchronized (cache) { + var cached = cache.get(id); + return cached != null ? cached : UNKNOWN; } - return cache; } - } - private static @NonNull Map load(@NonNull File buildDir) { - Map cache = new TreeMap<>(); - File storage = storageFor(buildDir); - if (storage.isFile()) { - try (Stream lines = Files.lines(storage.toPath(), StandardCharsets.UTF_8)) { - lines.forEach(line -> { - int idx = line.indexOf(' '); - if (idx == -1) { - return; - } + @Override + public void put(Job job, String id, Known target) { + File buildDir = job.getBuildDir(); + var cache = cacheFor(buildDir); + synchronized (cache) { + cache.put(id, target); + File storage = storageFor(buildDir); + LOGGER.fine(() -> "saving to " + storage + ": " + cache); + try (AtomicFileWriter cw = new AtomicFileWriter(storage)) { try { - cache.put(line.substring(0, idx), Integer.parseInt(line.substring(idx + 1))); - } catch (NumberFormatException x) { - LOGGER.log(Level.WARNING, "failed to read " + storage, x); + for (var entry : cache.entrySet()) { + cw.write(entry.getKey()); + cw.write(' '); + cw.write(Integer.toString(entry.getValue() instanceof Cache.Some some ? some.number : -1)); + cw.write('\n'); + } + cw.commit(); + } finally { + cw.abort(); } - }); - } catch (IOException x) { - LOGGER.log(Level.WARNING, "failed to read " + storage, x); + } catch (IOException x) { + LOGGER.log(Level.WARNING, "failed to update " + storage, x); + } } - LOGGER.fine(() -> "loading from " + storage + ": " + cache); } - return cache; - } - static @NonNull File storageFor(@NonNull File buildDir) { - return new File(buildDir, "permalinks"); - } + private @NonNull Map cacheFor(@NonNull File buildDir) { + synchronized (caches) { + var cache = caches.get(buildDir); + if (cache == null) { + cache = load(buildDir); + caches.put(buildDir, cache); + } + return cache; + } + } - /** - * Remembers the value 'n' in the cache for future {@link #resolve(Job)}. - */ - protected void updateCache(@NonNull Job job, @CheckForNull Run b) { - File buildDir = job.getBuildDir(); - Map cache = cacheFor(buildDir); - synchronized (cache) { - cache.put(getId(), b == null ? RESOLVES_TO_NONE : b.getNumber()); + private static @NonNull Map load(@NonNull File buildDir) { + Map cache = new TreeMap<>(); File storage = storageFor(buildDir); - LOGGER.fine(() -> "saving to " + storage + ": " + cache); - try (AtomicFileWriter cw = new AtomicFileWriter(storage)) { - try { - for (Map.Entry entry : cache.entrySet()) { - cw.write(entry.getKey()); - cw.write(' '); - cw.write(Integer.toString(entry.getValue())); - cw.write('\n'); - } - cw.commit(); - } finally { - cw.abort(); + if (storage.isFile()) { + try (Stream lines = Files.lines(storage.toPath(), StandardCharsets.UTF_8)) { + lines.forEach(line -> { + int idx = line.indexOf(' '); + if (idx == -1) { + return; + } + try { + int number = Integer.parseInt(line.substring(idx + 1)); + cache.put(line.substring(0, idx), number == -1 ? Cache.NONE : new Cache.Some(number)); + } catch (NumberFormatException x) { + LOGGER.log(Level.WARNING, "failed to read " + storage, x); + } + }); + } catch (IOException x) { + LOGGER.log(Level.WARNING, "failed to read " + storage, x); } - } catch (IOException x) { - LOGGER.log(Level.WARNING, "failed to update " + storage, x); + LOGGER.fine(() -> "loading from " + storage + ": " + cache); } + return cache; + } + + static @NonNull File storageFor(@NonNull File buildDir) { + return new File(buildDir, "permalinks"); } } @@ -380,7 +477,5 @@ public boolean apply(Run run) { @Restricted(NoExternalUse.class) public static void initialized() {} - private static final int RESOLVES_TO_NONE = -1; - private static final Logger LOGGER = Logger.getLogger(PeepholePermalink.class.getName()); } diff --git a/test/src/test/java/jenkins/model/PeepholePermalinkTest.java b/test/src/test/java/jenkins/model/PeepholePermalinkTest.java index 8d06ab980173..2a4e05854962 100644 --- a/test/src/test/java/jenkins/model/PeepholePermalinkTest.java +++ b/test/src/test/java/jenkins/model/PeepholePermalinkTest.java @@ -82,7 +82,7 @@ public void basics() throws Exception { } private void assertStorage(String id, Job job, Run build) throws Exception { - assertThat(Files.readAllLines(PeepholePermalink.storageFor(job.getBuildDir()).toPath(), StandardCharsets.UTF_8), + assertThat(Files.readAllLines(PeepholePermalink.DefaultCache.storageFor(job.getBuildDir()).toPath(), StandardCharsets.UTF_8), hasItem(id + " " + (build == null ? -1 : build.getNumber()))); } diff --git a/test/src/test/java/jenkins/model/lazy/LazyBuildMixInTest.java b/test/src/test/java/jenkins/model/lazy/LazyBuildMixInTest.java index 6b2ef4e6cf41..53f0603dd0b1 100644 --- a/test/src/test/java/jenkins/model/lazy/LazyBuildMixInTest.java +++ b/test/src/test/java/jenkins/model/lazy/LazyBuildMixInTest.java @@ -24,6 +24,9 @@ package jenkins.model.lazy; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertNull; @@ -87,6 +90,40 @@ public class LazyBuildMixInTest { assertEquals(b3, b1a.getNextBuild()); } + @Test public void numericLookups() throws Exception { + var p = r.createFreeStyleProject(); + var b1 = r.buildAndAssertSuccess(p); + var b2 = r.buildAndAssertSuccess(p); + var b3 = r.buildAndAssertSuccess(p); + var b4 = r.buildAndAssertSuccess(p); + var b5 = r.buildAndAssertSuccess(p); + var b6 = r.buildAndAssertSuccess(p); + b1.delete(); + b3.delete(); + b6.delete(); + // leaving 2, 4, 5 + assertThat(p.getFirstBuild(), is(b2)); + assertThat(p.getLastBuild(), is(b5)); + assertThat(p.getNearestBuild(-1), is(b2)); + assertThat(p.getNearestBuild(0), is(b2)); + assertThat(p.getNearestBuild(1), is(b2)); + assertThat(p.getNearestBuild(2), is(b2)); + assertThat(p.getNearestBuild(3), is(b4)); + assertThat(p.getNearestBuild(4), is(b4)); + assertThat(p.getNearestBuild(5), is(b5)); + assertThat(p.getNearestBuild(6), nullValue()); + assertThat(p.getNearestBuild(7), nullValue()); + assertThat(p.getNearestOldBuild(-1), nullValue()); + assertThat(p.getNearestOldBuild(0), nullValue()); + assertThat(p.getNearestOldBuild(1), nullValue()); + assertThat(p.getNearestOldBuild(2), is(b2)); + assertThat(p.getNearestOldBuild(3), is(b2)); + assertThat(p.getNearestOldBuild(4), is(b4)); + assertThat(p.getNearestOldBuild(5), is(b5)); + assertThat(p.getNearestOldBuild(6), is(b5)); + assertThat(p.getNearestOldBuild(7), is(b5)); + } + @Issue("JENKINS-20662") @Test public void newRunningBuildRelationFromPrevious() throws Exception { FreeStyleProject p = r.createFreeStyleProject();