Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize Label parsing calls #8395

Merged
merged 8 commits into from
Aug 23, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
14 changes: 12 additions & 2 deletions core/src/main/java/hudson/model/Node.java
Original file line number Diff line number Diff line change
Expand Up @@ -298,20 +298,30 @@ 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
jglick marked this conversation as resolved.
Show resolved Hide resolved
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));
Comment on lines -196 to +199
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caused JENKINS-71937, as label doesn't get set

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());
}
}
}