Skip to content

Commit

Permalink
Optimize Label parsing calls (jenkinsci#8395)
Browse files Browse the repository at this point in the history
* 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)

* Fix reviews

* Fix include

* Avoid parsing label every time getAssignedLabels is called

* Store Set<LabelAtom> at Node descendants level
* Compute it whenever labelString is updated

* Fix spotbugs / test issue

* Make it restricted
  • Loading branch information
Vlatombe authored and yaroslavafenkin committed Sep 5, 2023
1 parent c8c0cf3 commit 6ea5a73
Show file tree
Hide file tree
Showing 5 changed files with 156 additions and 9 deletions.
12 changes: 9 additions & 3 deletions core/src/main/java/hudson/model/Label.java
Original file line number Diff line number Diff line change
Expand Up @@ -592,10 +592,16 @@ public static Set<LabelAtom> parse(@CheckForNull String labels) {
final Set<LabelAtom> 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;
}

Expand Down
16 changes: 14 additions & 2 deletions core/src/main/java/hudson/model/Node.java
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
import net.sf.json.JSONObject;
import org.jvnet.localizer.Localizable;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
import org.kohsuke.accmod.restrictions.ProtectedExternally;
import org.kohsuke.stapler.BindInterceptor;
import org.kohsuke.stapler.Stapler;
Expand Down Expand Up @@ -298,20 +299,31 @@ public OfflineCause getTemporaryOfflineCause() {
public TagCloud<LabelAtom> getLabelCloud() {
return new TagCloud<>(getAssignedLabels(), Label::getTiedJobCount);
}

/**
* @return An immutable set of LabelAtom associated with the current node label.
*/
@NonNull
@Restricted(NoExternalUse.class)
protected Set<LabelAtom> 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
* assigned labels and dynamically assigned labels via the
* {@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<LabelAtom> getAssignedLabels() {
Set<LabelAtom> r = Label.parse(getLabelString());
Set<LabelAtom> r = new HashSet<>(getLabelAtomSet());
r.add(getSelfLabel());
r.addAll(getDynamicLabels());
return Collections.unmodifiableSet(r);
Expand Down
21 changes: 19 additions & 2 deletions core/src/main/java/hudson/model/Slave.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -179,6 +181,7 @@ protected Slave(@NonNull String name, String remoteFS, ComputerLauncher launcher
this.name = name;
this.remoteFS = remoteFS;
this.launcher = launcher;
this.labelAtomSet = Collections.unmodifiableSet(Label.parse(label));
}

/**
Expand All @@ -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();
this.labelAtomSet = Collections.unmodifiableSet(Label.parse(labelString));
this.launcher = launcher;
this.retentionStrategy = retentionStrategy;
getAssignedLabels(); // compute labels now
Expand Down Expand Up @@ -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<LabelAtom> labelAtomSet;

@Override
protected Set<LabelAtom> getLabelAtomSet() {
return labelAtomSet;
}

@Override
public Callable<ClockDifference, IOException> getClockDifferenceCallable() {
return new GetClockDifference1();
Expand Down Expand Up @@ -574,6 +590,7 @@ public int hashCode() {
protected Object readResolve() {
if (nodeProperties == null)
nodeProperties = new DescribableList<>(this);
_setLabelString(label);
return this;
}

Expand Down
34 changes: 32 additions & 2 deletions core/src/main/java/jenkins/model/Jenkins.java
Original file line number Diff line number Diff line change
Expand Up @@ -930,7 +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);

labelAtomSet = Collections.unmodifiableSet(Label.parse(label));
try {
dependencyGraph = DependencyGraph.EMPTY;
} catch (InternalError e) {
Expand Down Expand Up @@ -1088,6 +1088,7 @@ protected Object readResolve() {
/* deserializing without a value set means we need to migrate */
nodeRenameMigrationNeeded = true;
}
_setLabelString(label);

return this;
}
Expand Down Expand Up @@ -2114,6 +2115,20 @@ 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.
*/
@Restricted(NoExternalUse.class)
public @Nullable LabelAtom tryGetLabelAtom(@NonNull String name) {
Label label = labels.get(name);
if (label instanceof LabelAtom) {
return (LabelAtom) label;
}
return null;
}


/**
* Gets all the active labels in the current system.
*/
Expand All @@ -2126,6 +2141,14 @@ public Set<Label> getLabels() {
return r;
}

@NonNull
private transient Set<LabelAtom> labelAtomSet;

@Override
protected Set<LabelAtom> getLabelAtomSet() {
return labelAtomSet;
}

public Set<LabelAtom> getLabelAtoms() {
Set<LabelAtom> r = new TreeSet<>();
for (Label l : labels.values()) {
Expand Down Expand Up @@ -3291,10 +3314,17 @@ public String getLabelString() {

@Override
public void setLabelString(String label) throws IOException {
this.label = label;
_setLabelString(label);
save();
}

private void _setLabelString(String label) {
this.label = label;
if (Jenkins.getInstanceOrNull() != null) { // avoid on unit tests
this.labelAtomSet = Collections.unmodifiableSet(Label.parse(label));
}
}

@NonNull
@Override
public LabelAtom getSelfLabel() {
Expand Down
82 changes: 82 additions & 0 deletions test/src/test/java/hudson/model/labels/LabelBenchmarkTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package hudson.model.labels;

import static org.junit.Assert.assertTrue;

import hudson.model.Label;
import hudson.slaves.DumbSlave;
import hudson.slaves.JNLPLauncher;
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.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;
import org.openjdk.jmh.runner.options.OptionsBuilder;

public class LabelBenchmarkTest {
@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(LabelBenchmarkTest.class.getName() + ".*");
new Runner(options.build()).run();
assertTrue(Files.exists(Paths.get("jmh-report.json")));
}

@JmhBenchmark
public static class NodeLabelBenchmark {
public static class StateImpl extends JmhBenchmarkState {
@Override
public void setup() throws Exception {
DumbSlave test = new DumbSlave("test", "/tmp/slave", new JNLPLauncher());
test.setLabelString("a b c");
getJenkins().addNode(test);
}
}

@Benchmark
public void nodeGetAssignedLabels(StateImpl state, Blackhole blackhole) {
blackhole.consume(state.getJenkins().getNode("test").getAssignedLabels());
}
}


@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"));
}

@Benchmark
public void jenkinsGetAssignedLabels(MyState state, Blackhole blackhole) {
blackhole.consume(state.getJenkins().getAssignedLabels());
}
}
}

0 comments on commit 6ea5a73

Please sign in to comment.