From 0677dbea0873b6b94935909343d0b75154ad318f Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Fri, 18 Aug 2023 16:44:41 +0200 Subject: [PATCH 1/6] Optimize Label#parse for simple label For a simple label, this speeds up lookup of an existing simple label (no expression used): 16ns vs 106ns (6x faster) --- core/src/main/java/hudson/model/Label.java | 12 ++++-- core/src/main/java/jenkins/model/Jenkins.java | 10 +++++ .../hudson/model/labels/LabelBenchmark.java | 23 +++++++++++ .../model/labels/LabelBenchmarkTest.java | 38 +++++++++++++++++++ 4 files changed, 80 insertions(+), 3 deletions(-) create mode 100644 test/src/test/java/hudson/model/labels/LabelBenchmark.java create mode 100644 test/src/test/java/hudson/model/labels/LabelBenchmarkTest.java diff --git a/core/src/main/java/hudson/model/Label.java b/core/src/main/java/hudson/model/Label.java index 43604d408aac..b0c32f212a09 100644 --- a/core/src/main/java/hudson/model/Label.java +++ b/core/src/main/java/hudson/model/Label.java @@ -592,10 +592,16 @@ public static Set parse(@CheckForNull String labels) { final Set r = new TreeSet<>(); labels = fixNull(labels); if (labels.length() > 0) { - final QuotedStringTokenizer tokenizer = new QuotedStringTokenizer(labels); - while (tokenizer.hasMoreTokens()) - r.add(Jenkins.get().getLabelAtom(tokenizer.nextToken())); + Jenkins j = Jenkins.get(); + LabelAtom labelAtom = j.tryGetLabelAtom(labels); + if (labelAtom == null) { + final QuotedStringTokenizer tokenizer = new QuotedStringTokenizer(labels); + while (tokenizer.hasMoreTokens()) + r.add(j.getLabelAtom(tokenizer.nextToken())); + } else { + r.add(labelAtom); } + } return r; } diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index 9bf94cf9389a..1bba2320e98c 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -2114,6 +2114,16 @@ public Label getLabel(String expr) { } } + /** + * Returns the label atom of the given name, only if it already exists. + * @return non-null if the label atom already exists. + */ + public @Nullable LabelAtom tryGetLabelAtom(@CheckForNull String name) { + if (name == null) return null; + return (LabelAtom) labels.get(name); + } + + /** * Gets all the active labels in the current system. */ diff --git a/test/src/test/java/hudson/model/labels/LabelBenchmark.java b/test/src/test/java/hudson/model/labels/LabelBenchmark.java new file mode 100644 index 000000000000..2ff45eb55369 --- /dev/null +++ b/test/src/test/java/hudson/model/labels/LabelBenchmark.java @@ -0,0 +1,23 @@ +package hudson.model.labels; + +import hudson.model.Label; +import jenkins.benchmark.jmh.JmhBenchmark; +import jenkins.benchmark.jmh.JmhBenchmarkState; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.infra.Blackhole; + +@JmhBenchmark +public class LabelBenchmark { + public static class MyState extends JmhBenchmarkState { + } + + @Benchmark + public void simpleLabel(MyState state, Blackhole blackhole) { + blackhole.consume(Label.parse("some-label")); + } + + @Benchmark + public void complexLabel(MyState state, Blackhole blackhole) { + blackhole.consume(Label.parse("label1 && label2")); + } +} diff --git a/test/src/test/java/hudson/model/labels/LabelBenchmarkTest.java b/test/src/test/java/hudson/model/labels/LabelBenchmarkTest.java new file mode 100644 index 000000000000..e87e85987a8f --- /dev/null +++ b/test/src/test/java/hudson/model/labels/LabelBenchmarkTest.java @@ -0,0 +1,38 @@ +package hudson.model.labels; + +import static org.junit.Assert.assertTrue; + +import java.nio.file.Files; +import java.nio.file.Paths; +import java.util.concurrent.TimeUnit; +import org.junit.Ignore; +import org.junit.Test; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.results.format.ResultFormatType; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.options.ChainedOptionsBuilder; +import org.openjdk.jmh.runner.options.OptionsBuilder; + +public class LabelBenchmarkTest { + @Test + @Ignore(value="This is a benchmark, not a test") + public void runBenchmark() throws Exception { + // run the minimum possible number of iterations + ChainedOptionsBuilder options = new OptionsBuilder() + .mode(Mode.AverageTime) + .forks(1) + .result("jmh-report.json") + .resultFormat(ResultFormatType.JSON) + .operationsPerInvocation(1) + .threads(1) + .warmupForks(0) + .warmupIterations(0) + .measurementBatchSize(1) + .measurementIterations(1) + .timeUnit(TimeUnit.NANOSECONDS) + .shouldFailOnError(true) + .include(LabelBenchmark.class.getName() + ".*"); + new Runner(options.build()).run(); + assertTrue(Files.exists(Paths.get("jmh-report.json"))); + } +} From 557af9c769b7d7ca6307bf4a1aa3663b54b3a86d Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Mon, 21 Aug 2023 09:15:18 +0200 Subject: [PATCH 2/6] Fix reviews --- core/src/main/java/jenkins/model/Jenkins.java | 10 +++++--- .../hudson/model/labels/LabelBenchmark.java | 23 ------------------- .../model/labels/LabelBenchmarkTest.java | 23 ++++++++++++++++++- 3 files changed, 29 insertions(+), 27 deletions(-) delete mode 100644 test/src/test/java/hudson/model/labels/LabelBenchmark.java diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index 1bba2320e98c..52523d2e3d42 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -2118,9 +2118,13 @@ public Label getLabel(String expr) { * Returns the label atom of the given name, only if it already exists. * @return non-null if the label atom already exists. */ - public @Nullable LabelAtom tryGetLabelAtom(@CheckForNull String name) { - if (name == null) return null; - return (LabelAtom) labels.get(name); + @Restricted(NoExternalUse.class) + public @Nullable LabelAtom tryGetLabelAtom(@NonNull String name) { + Label label = labels.get(name); + if (label instanceof LabelAtom) { + return (LabelAtom) label; + } + return null; } diff --git a/test/src/test/java/hudson/model/labels/LabelBenchmark.java b/test/src/test/java/hudson/model/labels/LabelBenchmark.java deleted file mode 100644 index 2ff45eb55369..000000000000 --- a/test/src/test/java/hudson/model/labels/LabelBenchmark.java +++ /dev/null @@ -1,23 +0,0 @@ -package hudson.model.labels; - -import hudson.model.Label; -import jenkins.benchmark.jmh.JmhBenchmark; -import jenkins.benchmark.jmh.JmhBenchmarkState; -import org.openjdk.jmh.annotations.Benchmark; -import org.openjdk.jmh.infra.Blackhole; - -@JmhBenchmark -public class LabelBenchmark { - public static class MyState extends JmhBenchmarkState { - } - - @Benchmark - public void simpleLabel(MyState state, Blackhole blackhole) { - blackhole.consume(Label.parse("some-label")); - } - - @Benchmark - public void complexLabel(MyState state, Blackhole blackhole) { - blackhole.consume(Label.parse("label1 && label2")); - } -} diff --git a/test/src/test/java/hudson/model/labels/LabelBenchmarkTest.java b/test/src/test/java/hudson/model/labels/LabelBenchmarkTest.java index e87e85987a8f..3914dcaf7f50 100644 --- a/test/src/test/java/hudson/model/labels/LabelBenchmarkTest.java +++ b/test/src/test/java/hudson/model/labels/LabelBenchmarkTest.java @@ -2,12 +2,17 @@ import static org.junit.Assert.assertTrue; +import hudson.model.Label; import java.nio.file.Files; import java.nio.file.Paths; import java.util.concurrent.TimeUnit; +import jenkins.benchmark.jmh.JmhBenchmark; +import jenkins.benchmark.jmh.JmhBenchmarkState; import org.junit.Ignore; import org.junit.Test; +import org.openjdk.jmh.annotations.Benchmark; import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.infra.Blackhole; import org.openjdk.jmh.results.format.ResultFormatType; import org.openjdk.jmh.runner.Runner; import org.openjdk.jmh.runner.options.ChainedOptionsBuilder; @@ -15,7 +20,7 @@ public class LabelBenchmarkTest { @Test - @Ignore(value="This is a benchmark, not a test") + @Ignore(value = "This is a benchmark, not a test") public void runBenchmark() throws Exception { // run the minimum possible number of iterations ChainedOptionsBuilder options = new OptionsBuilder() @@ -35,4 +40,20 @@ public void runBenchmark() throws Exception { new Runner(options.build()).run(); assertTrue(Files.exists(Paths.get("jmh-report.json"))); } + + @JmhBenchmark + public static class LabelBenchmark { + public static class MyState extends JmhBenchmarkState { + } + + @Benchmark + public void simpleLabel(MyState state, Blackhole blackhole) { + blackhole.consume(Label.parse("some-label")); + } + + @Benchmark + public void complexLabel(MyState state, Blackhole blackhole) { + blackhole.consume(Label.parse("label1 && label2")); + } + } } From 26a8b8a2a540f3081ec9c2ca4e64aef5210d7e2c Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Mon, 21 Aug 2023 09:23:43 +0200 Subject: [PATCH 3/6] Fix include --- test/src/test/java/hudson/model/labels/LabelBenchmarkTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/src/test/java/hudson/model/labels/LabelBenchmarkTest.java b/test/src/test/java/hudson/model/labels/LabelBenchmarkTest.java index 3914dcaf7f50..3c5f8de95540 100644 --- a/test/src/test/java/hudson/model/labels/LabelBenchmarkTest.java +++ b/test/src/test/java/hudson/model/labels/LabelBenchmarkTest.java @@ -36,7 +36,7 @@ public void runBenchmark() throws Exception { .measurementIterations(1) .timeUnit(TimeUnit.NANOSECONDS) .shouldFailOnError(true) - .include(LabelBenchmark.class.getName() + ".*"); + .include(LabelBenchmark.class.getName().replace("$", ".") + ".*"); new Runner(options.build()).run(); assertTrue(Files.exists(Paths.get("jmh-report.json"))); } From 02f06cb028d643522bec849f9f58541056464455 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Fri, 18 Aug 2023 17:41:50 +0200 Subject: [PATCH 4/6] Avoid parsing label every time getAssignedLabels is called * Store Set at Node descendants level * Compute it whenever labelString is updated --- core/src/main/java/hudson/model/Node.java | 14 +++++++++++-- core/src/main/java/hudson/model/Slave.java | 21 +++++++++++++++++-- core/src/main/java/jenkins/model/Jenkins.java | 17 ++++++++++++++- .../model/labels/LabelBenchmarkTest.java | 7 +++++-- 4 files changed, 52 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/hudson/model/Node.java b/core/src/main/java/hudson/model/Node.java index 519d89c10970..1fedc66a5dd1 100644 --- a/core/src/main/java/hudson/model/Node.java +++ b/core/src/main/java/hudson/model/Node.java @@ -298,6 +298,16 @@ public OfflineCause getTemporaryOfflineCause() { public TagCloud getLabelCloud() { return new TagCloud<>(getAssignedLabels(), Label::getTiedJobCount); } + + /** + * @return An immutable set of LabelAtom associated with the current node label. + */ + @NonNull + protected Set getLabelAtomSet() { + // Default implementation doesn't cache, since we can't hook on label updates. + return Collections.unmodifiableSet(Label.parse(getLabelString())); + } + /** * Returns the possibly empty set of labels that are assigned to this node, * including the automatic {@link #getSelfLabel() self label}, manually @@ -305,13 +315,13 @@ public TagCloud getLabelCloud() { * {@link LabelFinder} extension point. * * This method has a side effect of updating the hudson-wide set of labels - * and should be called after events that will change that - e.g. a agent + * and should be called after events that will change that - e.g. an agent * connecting. */ @Exported public Set getAssignedLabels() { - Set r = Label.parse(getLabelString()); + Set r = new HashSet<>(getLabelAtomSet()); r.add(getSelfLabel()); r.addAll(getDynamicLabels()); return Collections.unmodifiableSet(r); diff --git a/core/src/main/java/hudson/model/Slave.java b/core/src/main/java/hudson/model/Slave.java index 22eb63cd2fc6..e2d3ea73d380 100644 --- a/core/src/main/java/hudson/model/Slave.java +++ b/core/src/main/java/hudson/model/Slave.java @@ -36,6 +36,7 @@ import hudson.Util; import hudson.cli.CLI; import hudson.model.Descriptor.FormException; +import hudson.model.labels.LabelAtom; import hudson.remoting.Callable; import hudson.remoting.Channel; import hudson.remoting.Which; @@ -60,6 +61,7 @@ import java.net.URLConnection; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.List; import java.util.Set; import java.util.jar.JarFile; @@ -179,6 +181,7 @@ protected Slave(@NonNull String name, String remoteFS, ComputerLauncher launcher this.name = name; this.remoteFS = remoteFS; this.launcher = launcher; + _setLabelString(label); } /** @@ -193,7 +196,7 @@ protected Slave(@NonNull String name, String nodeDescription, String remoteFS, i this.numExecutors = numExecutors; this.mode = mode; this.remoteFS = Util.fixNull(remoteFS).trim(); - this.label = Util.fixNull(labelString).trim(); + _setLabelString(labelString); this.launcher = launcher; this.retentionStrategy = retentionStrategy; getAssignedLabels(); // compute labels now @@ -328,11 +331,24 @@ public String getLabelString() { @Override @DataBoundSetter public void setLabelString(String labelString) throws IOException { - this.label = Util.fixNull(labelString).trim(); + _setLabelString(labelString); // Compute labels now. getAssignedLabels(); } + private void _setLabelString(String labelString) { + this.label = Util.fixNull(labelString).trim(); + this.labelAtomSet = Collections.unmodifiableSet(Label.parse(label)); + } + + @NonNull + private transient Set labelAtomSet; + + @Override + protected Set getLabelAtomSet() { + return labelAtomSet; + } + @Override public Callable getClockDifferenceCallable() { return new GetClockDifference1(); @@ -574,6 +590,7 @@ public int hashCode() { protected Object readResolve() { if (nodeProperties == null) nodeProperties = new DescribableList<>(this); + _setLabelString(label); return this; } diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index 52523d2e3d42..66bcba75274f 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -930,6 +930,7 @@ protected Jenkins(File root, ServletContext context, PluginManager pluginManager Trigger.timer = new java.util.Timer("Jenkins cron thread"); queue = new Queue(LoadBalancer.CONSISTENT_HASH); + _setLabelString(label); try { dependencyGraph = DependencyGraph.EMPTY; @@ -1088,6 +1089,7 @@ protected Object readResolve() { /* deserializing without a value set means we need to migrate */ nodeRenameMigrationNeeded = true; } + _setLabelString(label); return this; } @@ -2140,6 +2142,14 @@ public Set