Skip to content

Improve Queue.maintain performance by introducing cache for Util.isOverridden#26797

Merged
MarkEWaite merged 1 commit into
jenkinsci:masterfrom
sghill:perf/cache-is-overridden
May 25, 2026
Merged

Improve Queue.maintain performance by introducing cache for Util.isOverridden#26797
MarkEWaite merged 1 commit into
jenkinsci:masterfrom
sghill:perf/cache-is-overridden

Conversation

@sghill

@sghill sghill commented May 16, 2026

Copy link
Copy Markdown
Contributor

Fixes #26796

Adds a method to check the size so cache monitoring can be easily wired into observability tools. Also adds a method to clear cache so it could be called from Jenkins script console if necessary. Happy to add a feature flag or make other changes if desired.

Testing done

Automated tests added and mvn verify passed.

Screenshots (UI changes only)

N/A

Before

N/A

After

N/A

Proposed changelog entries

  • Cache calls to Util.is Overridden to improve Queue.maintain performance.

Proposed changelog category

/label internal

Proposed upgrade guidelines

N/A

Submitter checklist

  • The 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.
  • UI changes do not introduce regressions when enforcing the current default rules of Content Security Policy Plugin. In particular, 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.

Desired reviewers

@timja for being the initial reviewer on a similar change for this setup in #11032

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

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, be a Bug or Improvement, and either the issue or pull request must be labeled as lts-candidate to be considered.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This pull request aims to reduce Queue.maintain contention by caching Util.isOverridden results (avoiding repeated reflective lookups), and adds small APIs to observe and clear that cache.

Changes:

  • Add an isOverridden result cache in hudson.Util and route isOverridden through it.
  • Add cache observability/maintenance methods (isOverriddenCacheSize, clearIsOverriddenCache).
  • Add JUnit tests covering cache hit/miss behavior and cache keying.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
core/src/main/java/hudson/Util.java Introduces the isOverridden cache plus cache size/clear methods.
core/src/test/java/hudson/util/IsOverriddenTest.java Adds unit tests validating caching behavior and cache key composition.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1507 to +1532
// Keyed by (base, derived, methodName, paramTypes). Derived is held weakly so that plugin
// classloaders remain GC-eligible after unload. Results are stable for the lifetime of a
// classloader, so no invalidation is needed.
private static final ConcurrentHashMap<IsOverriddenCacheKey, Boolean> IS_OVERRIDDEN_CACHE =
new ConcurrentHashMap<>();

private record IsOverriddenCacheKey(
Class<?> base,
WeakReference<Class<?>> derivedRef,
String methodName,
List<Class<?>> types) {

@Override
public boolean equals(Object o) {
if (!(o instanceof IsOverriddenCacheKey other)) return false;
return base == other.base
&& derivedRef.get() == other.derivedRef.get()
&& methodName.equals(other.methodName)
&& types.equals(other.types);
}

@Override
public int hashCode() {
// Identity hash matches the identity comparison in equals.
return Objects.hash(base, System.identityHashCode(derivedRef.get()), methodName, types);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if/ once this is merged it should be possible to use caffeine if that is helpful: #26600

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review!

I can make that change, or a periodic task (hourly?) to call clear on the cache. Let me know if either are desired.

I wasn't too concerned without having expiry because it's very rare for me to unload a plugin without restarting Jenkins. I'm sure you'll have a better idea of how common this is across users of Jenkins, so happy to defer to you.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

tbh I don't think you can unload a plugin. May not be necessary

Comment thread core/src/main/java/hudson/Util.java

@timja timja left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems fine.

Did you notice any improvement in testing?

@timja timja requested a review from a team May 18, 2026 09:29
@sghill

sghill commented May 19, 2026

Copy link
Copy Markdown
Contributor Author

@timja I'm seeing a 96% reduction with the cache in a JMH Benchmark I ran on my laptop (3 forks, 5 warmup iterations, 10 measured iterations). I tried to create the conditions I saw issues with on the controller - about 1000 builds in the queue for FreeStyleProjects.

  • Java 21.0.11-temurin
  • OS: Fedora 43
  • CPU: i7-10710U @ 1.10 GHz w/ 6 cores
  • Arch: x86_64
  • Built with: mvn -pl war install -DskipTests -Dcheckstyle.skip -am
  • Tested with: mvn -pl test test -Dtest=IsOverriddenBenchmarkTest -DfailIfNoTests=false -Dcheckstyle.skip -am
Screenshot 2026-05-18 at 23 39 56

@sghill

sghill commented May 20, 2026

Copy link
Copy Markdown
Contributor Author

Is this the sort of change we might consider for a LTS backport?

@timja

timja commented May 20, 2026

Copy link
Copy Markdown
Member

possibly but its not likely to make the next LTS patch version and as long as its in soon-ish it'll be in the next LTS baseline

@MarkEWaite MarkEWaite left a comment

Copy link
Copy Markdown
Contributor

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. Please see the merge process documentation for more information about the merge process.

/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 May 24, 2026
@MarkEWaite MarkEWaite added the ath-successful This PR has successfully passed the full acceptance-test-harness suite label May 24, 2026
@MarkEWaite MarkEWaite added the pct-successful This PR has successfully passed the full plugin-compatibility-test suite label May 25, 2026
@MarkEWaite MarkEWaite merged commit cc0aad6 into jenkinsci:master May 25, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ath-successful This PR has successfully passed the full acceptance-test-harness suite internal pct-successful This PR has successfully passed the full plugin-compatibility-test suite ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve Queue.maintain performance by caching Util.isOverridden

4 participants