[FIXED JENKINS-34722] Performance improvement to not load all jobs #100

christ66 · 2016-05-11T23:25:08Z

Currently the BuildUtil.getDownstreamBuild will iterate through all of the downstream project builds even if the upstream build never executed the downstream job. This PR adds a system property which can limit the number of downstream builds to search. A unit test has also been added to confirm that the limit is properly being reached and does not exceed the max number of builds to search for when looking for downstream builds.

@reviewbybees

The property improves performance for obtaining downstream projects by allowing users to reduce the number of runs to load when searching for the correct upstream project.

ghost · 2016-05-11T23:26:46Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

christ66 · 2016-05-12T19:01:05Z

@jglick

The unit test I wrote fails with the following:

java.lang.AssertionError: too many build records loaded from test1
    at org.jvnet.hudson.test.RunLoadCounter$Marker.onLoad(RunLoadCounter.java:119)
    at hudson.model.Run.onLoad(Run.java:355)
    at hudson.model.RunMap.retrieve(RunMap.java:225)
    at hudson.model.RunMap.retrieve(RunMap.java:57)
    at jenkins.model.lazy.AbstractLazyLoadRunMap.load(AbstractLazyLoadRunMap.java:465)
    at jenkins.model.lazy.AbstractLazyLoadRunMap.load(AbstractLazyLoadRunMap.java:448)
    at jenkins.model.lazy.AbstractLazyLoadRunMap.getByNumber(AbstractLazyLoadRunMap.java:356)
    at jenkins.model.lazy.AbstractLazyLoadRunMap.search(AbstractLazyLoadRunMap.java:332)
    at jenkins.model.lazy.LazyLoadRunMapEntrySet$1.next(LazyLoadRunMapEntrySet.java:74)
    at jenkins.model.lazy.LazyLoadRunMapEntrySet$1.next(LazyLoadRunMapEntrySet.java:63)
    at java.util.AbstractMap$2$1.next(AbstractMap.java:385)
    at hudson.util.Iterators$9.fetch(Iterators.java:383)
    at hudson.util.Iterators$9.hasNext(Iterators.java:369)
    at au.com.centrumsystems.hudson.plugin.util.BuildUtil.getDownstreamBuild(BuildUtil.java:71)
    at au.com.centrumsystems.hudson.plugin.util.BuildUtilTest$1.call(BuildUtilTest.java:81)
    at au.com.centrumsystems.hudson.plugin.util.BuildUtilTest$1.call(BuildUtilTest.java:78)
    at org.jvnet.hudson.test.RunLoadCounter.assertMaxLoads(RunLoadCounter.java:93)
    at au.com.centrumsystems.hudson.plugin.util.BuildUtilTest.testGetDownstreamBuildWithLimit(BuildUtilTest.java:78)

however when I run the unit test in debug mode I get the following:

loaded test1 #5
loaded test1 #4
loaded test1 #3
loaded test1 #3
loaded test1 #2
loaded test1 #1

which it looks like it loads the same job twice, and loads all of the builds. Interestingly the test passes when I use the debugger and fails without it.

jglick · 2016-05-12T21:58:42Z

src/main/java/au/com/centrumsystems/hudson/plugin/util/BuildUtil.java

-import hudson.model.AbstractBuild;
-import hudson.model.AbstractProject;
-import hudson.model.Cause;
+import hudson.model.*;


BTW we typically discourage star imports.

In fact Checkstyle is failing the build in this case.

jglick · 2016-05-12T22:23:40Z

It passes for me if I do

diff --git a/src/test/java/au/com/centrumsystems/hudson/plugin/util/BuildUtilTest.java b/src/test/java/au/com/centrumsystems/hudson/plugin/util/BuildUtilTest.java
index 6869bb5..ba7177d 100644
--- a/src/test/java/au/com/centrumsystems/hudson/plugin/util/BuildUtilTest.java
+++ b/src/test/java/au/com/centrumsystems/hudson/plugin/util/BuildUtilTest.java
@@ -53,7 +53,7 @@ public class BuildUtilTest extends HudsonTestCase {
     @Test
     public void testGetDownstreamBuildWithLimit() throws Exception {
         int max_upstream_depth = 3;
-        int total_proj2_builds = 5;
+        int total_proj2_builds = 10;
         System.setProperty(BuildUtil.class.getCanonicalName() + ".MAX_DOWNSTREAM_DEPTH", String.valueOf(max_upstream_depth));

         final FreeStyleProject proj1 = createFreeStyleProject();
@@ -75,7 +75,7 @@ public class BuildUtilTest extends HudsonTestCase {
         assertEquals("Proj2 does not have the correct number of builds.", total_proj2_builds + 1, proj2.getNextBuildNumber());
         RunLoadCounter.prepare(proj2);

-        assertFalse(RunLoadCounter.assertMaxLoads(proj2, max_upstream_depth - 1, new Callable<Boolean>() {
+        assertFalse(RunLoadCounter.assertMaxLoads(proj2, max_upstream_depth + 2, new Callable<Boolean>() {
             @Override
             public Boolean call() throws Exception {
                 return BuildUtil.getDownstreamBuild(proj2, freeStyleBuild) != null;

s/5/10/ to make sure we are not too close to the end of the builds anyway; and s/-1/+2/ to give us a little extra room for fencepost conditions.

christ66 · 2016-05-14T00:11:30Z

Thanks @jglick for the help fixing the unit test. It now passes on my local machine.

jglick · 2016-05-18T17:00:59Z

Still complaining about CheckStyle, sigh.

src/main/java/au/com/centrumsystems/hudson/plugin/util/BuildUtil.java:27: Using the '.*' form of import should be avoided - hudson.model.*.
src/main/java/au/com/centrumsystems/hudson/plugin/util/BuildUtil.java:59:17: Name 'max_downstream_depth' must match pattern '^[a-z][a-zA-Z0-9]*$'.
src/main/java/au/com/centrumsystems/hudson/plugin/util/BuildUtil.java:59:17: Variable 'max_downstream_depth' should be declared final.
src/main/java/au/com/centrumsystems/hudson/plugin/util/BuildUtil.java:66: Line is longer than 140 characters (found 149).
src/main/java/au/com/centrumsystems/hudson/plugin/util/BuildUtil.java:70: Line is longer than 140 characters (found 149).

christ66 · 2016-05-18T17:11:04Z

src/main/java/au/com/centrumsystems/hudson/plugin/util/BuildUtil.java

-    public static Action getAllBuildParametersAction(//
-            final AbstractBuild<?, ?> upstreamBuild, final AbstractProject<?, ?> downstreamProject) { //
+    public static Action getAllBuildParametersAction(final AbstractBuild<?, ?> upstreamBuild,
+                                                     final AbstractProject<?, ?> downstreamProject) {


This was not part of my initial commit, but checkstyle gave me a warning about this being over 140 characters.

christ66 · 2016-05-18T17:12:35Z

@jglick fixed checkstyle issues.

jglick · 2016-05-18T17:57:03Z

🐝

dalvizu · 2016-05-19T19:49:11Z

👍 thanks @christ66 !

recena · 2016-05-23T08:07:24Z

@dalvizu 👍 for cutting a new release with this PR.

dalvizu · 2016-05-23T18:35:30Z

Can I get a quick review on #101 before a release?

dalvizu · 2016-05-23T21:34:43Z

Fixed in 1.5.3.1

recena · 2016-05-24T08:14:11Z

@dalvizu Why 1.5.3.1 instead of 1.5.3? Please, before to take this kind of decisions, we should talk.

dalvizu · 2016-05-24T15:01:35Z

1.5.3.1 and 1.5.3 are the same - I botched the 1.5.3 release as the build requires jdk8 to build now or there is some javadoc error which I wasn't aware of. This isn't the beginning of a different versioning scheme or anything, 1.5.4 is the next release :)

christ66 added 2 commits May 11, 2016 02:25

Add property to reduce the max number of runs to load.

b1eaa19

The property improves performance for obtaining downstream projects by allowing users to reduce the number of runs to load when searching for the correct upstream project.

Change system property to MAX_DOWNSTREAM_DEPTH, and add a unit test.

0551cd1

jglick reviewed May 12, 2016
View reviewed changes

Fix unit test.

f88396a

Fix checkstyle errors.

0c5b51d

christ66 reviewed May 18, 2016
View reviewed changes

christ66 changed the title ~~Performance improvement to not load all jobs~~ [FIXED JENKINS-34722] Performance improvement to not load all jobs May 18, 2016

dalvizu merged commit 00a6a4f into jenkinsci:master May 20, 2016

christ66 deleted the perf branch May 20, 2016 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIXED JENKINS-34722] Performance improvement to not load all jobs #100

[FIXED JENKINS-34722] Performance improvement to not load all jobs #100

christ66 commented May 11, 2016 •

edited by jglick

ghost commented May 11, 2016

christ66 commented May 12, 2016

jglick May 12, 2016

jglick May 12, 2016

jglick commented May 12, 2016

christ66 commented May 14, 2016

jglick commented May 18, 2016

christ66 May 18, 2016

christ66 commented May 18, 2016

jglick commented May 18, 2016

dalvizu commented May 19, 2016

recena commented May 23, 2016 •

edited

dalvizu commented May 23, 2016

dalvizu commented May 23, 2016

recena commented May 24, 2016

dalvizu commented May 24, 2016

[FIXED JENKINS-34722] Performance improvement to not load all jobs #100

[FIXED JENKINS-34722] Performance improvement to not load all jobs #100

Conversation

christ66 commented May 11, 2016 • edited by jglick

ghost commented May 11, 2016

christ66 commented May 12, 2016

jglick May 12, 2016

Choose a reason for hiding this comment

jglick May 12, 2016

Choose a reason for hiding this comment

jglick commented May 12, 2016

christ66 commented May 14, 2016

jglick commented May 18, 2016

christ66 May 18, 2016

Choose a reason for hiding this comment

christ66 commented May 18, 2016

jglick commented May 18, 2016

dalvizu commented May 19, 2016

recena commented May 23, 2016 • edited

dalvizu commented May 23, 2016

dalvizu commented May 23, 2016

recena commented May 24, 2016

dalvizu commented May 24, 2016

christ66 commented May 11, 2016 •

edited by jglick

recena commented May 23, 2016 •

edited