Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-20052

ListView.getItems makes too many passes through jobs list

    XMLWordPrintable

Details

    • Jenkins 2.239

    Description

      Since pull 757 in 1.512, ListView can include items in subfolders. Unfortunately it seems that getItems calls ViewJobFilter with anomalous arguments: items may be a superset, rather than a subset, of allItems.

      Should be better tested, and simplified by inlining the includeItems method, which could also improve performance by removing the double traversal of the job hierarchy (which checks ACLs each time through).

      Attachments

        Issue Links

          Activity

            danielbeck Daniel Beck added a comment -

            Didn't JENKINS-18721 resolve the performance issue sufficiently?

            danielbeck Daniel Beck added a comment - Didn't JENKINS-18721 resolve the performance issue sufficiently?
            jglick Jesse Glick added a comment -

            Looks like JENKINS-18721 made some improvements, but getItems still looks to go through all jobs twice: once in includeItems, once in the loop to translate names into items.

            Basically the issue is that instead of doing the obvious thing—looking up explicitly-specified names using the method that exists for that purpose, then scanning recursive items whose relative name match the regexp and adding those in—the view is scanning recursive items, adding them to the name list, then going back and scanning recursive items again and collecting those whose (relative) names match the list. And the job filters are reported to be called with the wrong arguments: the direct children of the folder.

            jglick Jesse Glick added a comment - Looks like JENKINS-18721 made some improvements, but getItems still looks to go through all jobs twice: once in includeItems , once in the loop to translate names into items . Basically the issue is that instead of doing the obvious thing—looking up explicitly-specified names using the method that exists for that purpose, then scanning recursive items whose relative name match the regexp and adding those in—the view is scanning recursive items, adding them to the name list, then going back and scanning recursive items again and collecting those whose (relative) names match the list. And the job filters are reported to be called with the wrong arguments: the direct children of the folder.
            jglick Jesse Glick added a comment -

            JENKINS-20143 should have fixed the behavioral issue with recurse (though there is no test yet), but the poor implementation remains (and the code is even more bloated with the new expand method which yet again traverses the job hierarchy).

            jglick Jesse Glick added a comment - JENKINS-20143 should have fixed the behavioral issue with recurse (though there is no test yet), but the poor implementation remains (and the code is even more bloated with the new expand method which yet again traverses the job hierarchy).
            jglick Jesse Glick added a comment -

            Worth investigating whether the three existing fields in ListView controlling job inclusion—jobNames, includeRegex, and statusFilter—can all be migrated to be ViewJobFilter implementations. (recurse cannot, since it determines the all parameter to filters.)

            jglick Jesse Glick added a comment - Worth investigating whether the three existing fields in ListView controlling job inclusion— jobNames , includeRegex , and statusFilter —can all be migrated to be ViewJobFilter implementations. ( recurse cannot, since it determines the all parameter to filters.)
            danielbeck Daniel Beck added a comment -

            A great side-effect of making jobNames into a ViewJobFilter would probably be that jobs are no longer automatically added to the view the user was in when she created the job. Which is really annoying if you have views with very specific, computed contents.

            danielbeck Daniel Beck added a comment - A great side-effect of making jobNames into a ViewJobFilter would probably be that jobs are no longer automatically added to the view the user was in when she created the job. Which is really annoying if you have views with very specific, computed contents.
            jglick Jesse Glick added a comment -

            jobs [would no longer be] automatically added to the view the user was in when she created the job

            Right, unless you actually had one of the new filters configured, which I guess would happen only if jobNames were originally nonempty. A good point.

            jglick Jesse Glick added a comment - jobs [would no longer be] automatically added to the view the user was in when she created the job Right, unless you actually had one of the new filters configured, which I guess would happen only if jobNames were originally nonempty. A good point.
            pwiseman Peter Wiseman added a comment -

            Noting that change ([PR-4466|https://github.com/jenkinsci/jenkins/pull/4466]) results in Views with the StatusFilter set being removed when upgrading from <2.239 to >2.239.

            Related issues: JENKINS-62661JENKINS-63786.

            pwiseman Peter Wiseman added a comment - Noting that change ([PR-4466| https://github.com/jenkinsci/jenkins/pull/4466 ]) results in Views with the StatusFilter set being removed when upgrading from <2.239 to >2.239. Related issues:  JENKINS-62661 ,  JENKINS-63786 .

            People

              raihaan Raihaan Shouhell
              jglick Jesse Glick
              Votes:
              3 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: