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

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

Merged
merged 4 commits into from May 20, 2016

Conversation

christ66
Copy link
Member

@christ66 christ66 commented May 11, 2016

JENKINS-34722

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.
@reviewbybees
Copy link

@reviewbybees reviewbybees commented May 11, 2016

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
Copy link
Member Author

@christ66 christ66 commented May 12, 2016

@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.

import hudson.model.AbstractBuild;
import hudson.model.AbstractProject;
import hudson.model.Cause;
import hudson.model.*;
Copy link
Member

@jglick jglick May 12, 2016

Choose a reason for hiding this comment

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

BTW we typically discourage star imports.

Copy link
Member

@jglick jglick May 12, 2016

Choose a reason for hiding this comment

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

In fact Checkstyle is failing the build in this case.

@jglick
Copy link
Member

@jglick jglick commented May 12, 2016

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
Copy link
Member Author

@christ66 christ66 commented May 14, 2016

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

@jglick
Copy link
Member

@jglick jglick commented May 18, 2016

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).

public static Action getAllBuildParametersAction(//
final AbstractBuild<?, ?> upstreamBuild, final AbstractProject<?, ?> downstreamProject) { //
public static Action getAllBuildParametersAction(final AbstractBuild<?, ?> upstreamBuild,
final AbstractProject<?, ?> downstreamProject) {
Copy link
Member Author

@christ66 christ66 May 18, 2016

Choose a reason for hiding this comment

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

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

@christ66
Copy link
Member Author

@christ66 christ66 commented May 18, 2016

@jglick fixed checkstyle issues.

@christ66 christ66 changed the title Performance improvement to not load all jobs [FIXED JENKINS-34722] Performance improvement to not load all jobs May 18, 2016
@jglick
Copy link
Member

@jglick jglick commented May 18, 2016

🐝

@dalvizu
Copy link
Member

@dalvizu dalvizu commented May 19, 2016

👍 thanks @christ66 !

@dalvizu dalvizu merged commit 00a6a4f into jenkinsci:master May 20, 2016
1 check passed
@christ66 christ66 deleted the perf branch May 20, 2016
@recena
Copy link

@recena recena commented May 23, 2016

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

@dalvizu
Copy link
Member

@dalvizu dalvizu commented May 23, 2016

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

@dalvizu
Copy link
Member

@dalvizu dalvizu commented May 23, 2016

Fixed in 1.5.3.1

@recena
Copy link

@recena recena commented May 24, 2016

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

@dalvizu
Copy link
Member

@dalvizu dalvizu commented May 24, 2016

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants