Skip to content

Optimize MappingWorksheet to avoid calling Task.getEstimatedDuration#11293

Merged
MarkEWaite merged 1 commit into
jenkinsci:masterfrom
jglick:MappingWorksheet
Nov 15, 2025
Merged

Optimize MappingWorksheet to avoid calling Task.getEstimatedDuration#11293
MarkEWaite merged 1 commit into
jenkinsci:masterfrom
jglick:MappingWorksheet

Conversation

@jglick
Copy link
Copy Markdown
Member

@jglick jglick commented Nov 13, 2025

Amends the rather old 0ff811f by trying to avoid calling Queue.Task.getEstimatedDuration() unless it is actually helpful to do so. Follows up my #7998 which did make this call faster in many cases; still, flame graphs show that in a heavily loaded controller, a major source of contention on the Queue lock is the MappingWorksheet constructor and the calculations it performs.

  • Quite often, this code is entered when there are no JobOffers at all, so the loop will not run and there is no purpose in even asking for the estimated duration.
  • When using cloud agents,
    * When Hudson makes a scheduling decision, Hudson considers predicted future load
    * — e.g., "We do currently have one available executor, but we know we need this for something else in 30 minutes,
    * so we can't currently schedule a build that takes 1 hour."
    simply makes no sense. Each item added to the queue triggers creation of an agent which will run its task. There is no reason to waste time doing anything fancier.
  • When at least one permanent agent is under consideration for the item, the more expensive historical behavior is preserved, in case someone finds it useful. (It seems dubious to me.) I am treating DumbSlave as the definition of permanent agent; use of
    * Partial implementation of {@link Slave} to be used by {@link AbstractCloudImpl}.
    is optional (see Use AbstractCloudSlave and AbstractCloudComputer ec2-plugin#1015 for example).

Testing done

Original performance problem observed sometimes under load in a CloudBees CI high availability controller. The patch appears to help, though running a controlled experiment is difficult.

So for only tested semi-interactively using functional tests in CloudBees CI representing various scenarios of cloud agents only, permanent agents only, or a mixture but the specific queue item is requesting a cloud agent label, and confirming that the appropriate FINE log message is printed. Other than the fairly low-level mock test, I did not find any existing functional tests that would confirm the purpose and effectiveness of the MappingWorksheet logic in the context of a real Queue.maintain.

Proposed changelog entries

  • Performance improvement for queue item scheduling.

Proposed changelog category

/label rfe

Proposed upgrade guidelines

N/A

Maintainer checklist

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

@comment-ops-bot comment-ops-bot Bot added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Nov 13, 2025
Copy link
Copy Markdown
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

🤷 this is the first time I even came across this class.

Copy link
Copy Markdown
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.

/label ready-for-merge

@comment-ops-bot 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 Nov 14, 2025
@MarkEWaite MarkEWaite merged commit 6e2067e into jenkinsci:master Nov 15, 2025
19 checks passed
@jglick jglick deleted the MappingWorksheet branch November 17, 2025 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants