Optimize Label parsing calls #8395

Vlatombe · 2023-08-18T15:07:14Z

Optimize Label parsing related calls

Node#getAssignedLabels parse Labels from scratch on every call. This ends up being called a lot via Jenkins#trimLabels. Caching the set of LabelAtom allows to parse labels only if the label string is updated.
Label#parse has come up in performance testing involving a lot of cloud agents using one-shot labels.

This speeds up lookup of an existing simple label (no expression used): 16 ns vs. 106 ns (6x faster)

Node#getAssignedLabels

LabelBenchmark.jenkinsGetAssignedLabels : 34ns vs. 41ns, expected since Jenkins doesn't have any assigned label by default.
NodeLabelBenchmark.nodeGetAssignedLabels : 91ns vs. 185ns (2x faster)

Testing done

Proposed changelog entries

Performance optimizations on Label parsing.

Proposed upgrade guidelines

N/A

Submitter checklist

Give feedback

The Jira issue, if it exists, is well-described.
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
There is automated testing or an explanation as to why this change has no tests.
New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
For dependency updates, there are links to external changelogs and, if possible, full differentials.
For new APIs and extension points, there is a link to at least one consumer.
Options

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Give feedback

There are at least two (2) approvals for the pull request and no outstanding requests for change.
Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Proper changelog labels are set so that the changelog can be generated automatically.
If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).
Options

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/jenkins/model/Jenkins.java

test/src/test/java/hudson/model/labels/LabelBenchmark.java

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

NotMyFault

Spotbugs and the failure in hudson.XmlFileTest.canReadXmlWithControlCharsTest are left to address

NotMyFault

Thanks!

timja · 2023-08-21T14:51:36Z

/label ready-for-merge

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

core/src/main/java/hudson/model/Node.java

MarkEWaite · 2023-08-24T11:48:39Z

@Vlatombe thanks for this improvement. It appears that the improvement causes one of the automated tests in the workflow durable task plugin to fail as shown in https://github.com/jenkinsci/bom/pull/2412/checks?check_run_id=16175702436

Thanks to @daniel-beck for detecting the issue and mentioning it in jenkinsci/bom#2412 (comment)

Can you investigate that failure and propose a fix for the tests in the workflow durable task plugin so that a new version of workflow durable task plugin can be included in the plugin bill of materials? Without that fix, the plugin compatibility tests used for the plugin bill of materials will fail and block the next release of the plugin bill of materials.

Vlatombe · 2023-08-24T11:52:07Z

Hi @MarkEWaite this test failure has been addressed already - jenkinsci/workflow-durable-task-step-plugin#335

MarkEWaite · 2023-08-24T12:23:23Z

Thanks very much @Vlatombe ! I've triggered a dependabot update of the plugin bill of materials to bring the new release of the plugin into the BOM.

aaronmell · 2023-08-30T16:15:37Z

FYI, this change likely broken our Jenkins instance. Jenkins fails to start after upgrading, and we are getting a null reference error

SEVERE        jenkins.InitReactorRunner$1#onTaskFailed: Failed Loading global config
Error
java.lang.NullPointerException
	at java.base/java.util.HashSet.<init>(HashSet.java:119)
	at hudson.model.Node.getAssignedLabels(Node.java:326)
	at jenkins.model.Jenkins.lambda$trimLabels$3(Jenkins.java:2323)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
	at jenkins.model.Jenkins.trimLabels(Jenkins.java:2323)
	at jenkins.model.Jenkins.trimLabels(Jenkins.java:2303)
	at jenkins.model.Nodes$6.run(Nodes.java:352)
	at hudson.model.Queue._withLock(Queue.java:1397)
	at hudson.model.Queue.withLock(Queue.java:1271)
	at jenkins.model.Nodes.load(Nodes.java:346)
	at jenkins.model.Jenkins$12.run(Jenkins.java:3488)
	at org.jvnet.hudson.reactor.TaskGraphBuilder$TaskImpl.run(TaskGraphBuilder.java:177)
	at org.jvnet.hudson.reactor.Reactor.runTask(Reactor.java:305)
	at jenkins.model.Jenkins$5.runTask(Jenkins.java:1170)
	at org.jvnet.hudson.reactor.Reactor$2.run(Reactor.java:221)
	at org.jvnet.hudson.reactor.Reactor$Node.run(Reactor.java:120)
	at jenkins.security.ImpersonatingExecutorService$1.run(ImpersonatingExecutorService.java:68)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:829)
Caused: org.jvnet.hudson.reactor.ReactorException
	at org.jvnet.hudson.reactor.Reactor.execute(Reactor.java:290)
	at jenkins.InitReactorRunner.run(InitReactorRunner.java:49)
	at jenkins.model.Jenkins.executeReactor(Jenkins.java:1205)
	at jenkins.model.Jenkins.<init>(Jenkins.java:992)
	at hudson.model.Hudson.<init>(Hudson.java:86)
	at hudson.model.Hudson.<init>(Hudson.java:82)
	at hudson.WebAppMain$3.run(WebAppMain.java:247)
Caused: hudson.util.HudsonFailedToLoad
	at hudson.WebAppMain$3.run(WebAppMain.java:264)

aaronmell · 2023-08-30T16:59:48Z

We got it to start by removing all of the nodes in the nodes folder, however after restarting it crashes again

All of our nodes have labels, and the only nodes running at the time were EC2 @Vlatombe

Vlatombe · 2023-08-31T07:37:18Z

Filed jenkinsci/ec2-plugin#883 to fix the impacted plugin. Given this happened already with 2 plugins overriding readResolve without calling its super implementation, I will also file a robustness improvement for this change.

Vlatombe · 2023-08-31T08:13:37Z

Filed #8448

* 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

Vlatombe · 2023-10-04T14:55:02Z

core/src/main/java/hudson/model/Slave.java

-        this.label = Util.fixNull(labelString).trim();
+        this.labelAtomSet = Collections.unmodifiableSet(Label.parse(labelString));


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

Got broken in jenkinsci#8395

[JENKINS-71937] Fix deprecated Slave constructor Got broken in #8395 Co-authored-by: Basil Crow <[email protected]>

[JENKINS-71937] Fix deprecated Slave constructor Got broken in jenkinsci#8395 Co-authored-by: Basil Crow <[email protected]> (cherry picked from commit 6fdfdd0)

Optimize Label#parse for simple label

0677dbe

For a simple label, this speeds up lookup of an existing simple label (no expression used): 16ns vs 106ns (6x faster)

Vlatombe marked this pull request as draft August 18, 2023 15:07

Vlatombe requested a review from jglick August 18, 2023 15:08

jglick reviewed Aug 18, 2023

View reviewed changes

timja added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Aug 19, 2023

Vlatombe added 4 commits August 21, 2023 09:16

Fix reviews

557af9c

Fix include

26a8b8a

Merge branch 'master' into label-parse-optimization

484224e

Avoid parsing label every time getAssignedLabels is called

02f06cb

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

Vlatombe marked this pull request as ready for review August 21, 2023 08:23

NotMyFault added the needs-test-fix One or more test case(s) need to be updated label Aug 21, 2023

NotMyFault reviewed Aug 21, 2023

View reviewed changes

Fix spotbugs / test issue

af2982a

Vlatombe changed the title ~~Optimize Label#parse for simple label~~ Optimize Label parsing calls Aug 21, 2023

NotMyFault removed the needs-test-fix One or more test case(s) need to be updated label Aug 21, 2023

NotMyFault approved these changes Aug 21, 2023

View reviewed changes

NotMyFault requested review from jglick and a team August 21, 2023 13:55

jglick approved these changes Aug 21, 2023

View reviewed changes

comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Aug 21, 2023

Merge branch 'master' into label-parse-optimization

a79e49b

jglick reviewed Aug 21, 2023

View reviewed changes

core/src/main/java/hudson/model/Node.java Show resolved Hide resolved

Make it restricted

c70bef4

jglick approved these changes Aug 22, 2023

View reviewed changes

timja merged commit 76bfee2 into jenkinsci:master Aug 23, 2023

Vlatombe deleted the label-parse-optimization branch August 23, 2023 07:11

jglick mentioned this pull request Aug 24, 2023

Test https://github.com/jenkinsci/jenkins/pull/8426 jenkinsci/bom#2412

Closed

Vlatombe mentioned this pull request Aug 31, 2023

Label parsing robustness fix #8448

Merged

jglick mentioned this pull request Sep 14, 2023

Fixing Pipeline resumption jenkinsci/docker-swarm-plugin#126

Merged

Vlatombe commented Oct 4, 2023

View reviewed changes

Vlatombe added a commit to Vlatombe/jenkins that referenced this pull request Oct 4, 2023

[JENKINS-71937] Fix deprecated Slave constructor

94d3b76

Got broken in jenkinsci#8395

Vlatombe mentioned this pull request Oct 4, 2023

[JENKINS-71937] Fix deprecated Slave constructor #8564

Merged

MarkEWaite pushed a commit that referenced this pull request Oct 6, 2023

[JENKINS-71937] Fix deprecated Slave constructor (#8564)

6fdfdd0

[JENKINS-71937] Fix deprecated Slave constructor Got broken in #8395 Co-authored-by: Basil Crow <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize Label parsing calls #8395

Optimize Label parsing calls #8395

Vlatombe commented Aug 18, 2023 •

edited by NotMyFault

Loading

Submitter checklist

Maintainer checklist

NotMyFault left a comment

NotMyFault left a comment

timja commented Aug 21, 2023

MarkEWaite commented Aug 24, 2023

Vlatombe commented Aug 24, 2023

MarkEWaite commented Aug 24, 2023

aaronmell commented Aug 30, 2023 •

edited

Loading

aaronmell commented Aug 30, 2023 •

edited

Loading

Vlatombe commented Aug 31, 2023

Vlatombe commented Aug 31, 2023

Vlatombe Oct 4, 2023

		this.label = Util.fixNull(labelString).trim();
		this.labelAtomSet = Collections.unmodifiableSet(Label.parse(labelString));

Optimize Label parsing calls #8395

Optimize Label parsing calls #8395

Conversation

Vlatombe commented Aug 18, 2023 • edited by NotMyFault Loading

Testing done

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

NotMyFault left a comment

Choose a reason for hiding this comment

NotMyFault left a comment

Choose a reason for hiding this comment

timja commented Aug 21, 2023

MarkEWaite commented Aug 24, 2023

Vlatombe commented Aug 24, 2023

MarkEWaite commented Aug 24, 2023

aaronmell commented Aug 30, 2023 • edited Loading

aaronmell commented Aug 30, 2023 • edited Loading

Vlatombe commented Aug 31, 2023

Vlatombe commented Aug 31, 2023

Vlatombe Oct 4, 2023

Choose a reason for hiding this comment

Vlatombe commented Aug 18, 2023 •

edited by NotMyFault

Loading

aaronmell commented Aug 30, 2023 •

edited

Loading

aaronmell commented Aug 30, 2023 •

edited

Loading