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

Use CompletableFuture in JarCache #715

Merged
merged 1 commit into from Jan 8, 2024
Merged

Conversation

basil
Copy link
Member

@basil basil commented Jan 6, 2024

When reading this code recently, I noticed we were using some custom classes like AsyncFutureImpl and FutureAdapter which now have equivalents in the Java Platform. I thought the code was easier to read when using the standard classes rather than our custom ones.

This is a tiny public API change for hudson.remoting.JarCache and a handful of package-private internal classes, but I searched in jenkinsci and cloudbees and found no consumers outside of Remoting itself, so this change seems safe from an API compatibility point of view.

I stepped through the code in the debugger before and after and found one minor change: when downloading a JAR for the first time, the chained future to read the file into memory from ResourceImageInJar runs immediately as part of the promise.complete(url) call in JarCacheSupport on the AtmostOneThreadExecutor thread rather than before where it was run lazily a little bit later when calling .get() from RemoteClassLoader#loadRemoteClass as part of the request handling thread. This doesn't seem like it should make any difference to me, and I think the readability benefits of using CompletableFuture everywhere are worth the minor change in behavior. But if people feel this is too risky, I can always roll back the call to CompletableFuture#thenApply in ResourceImageInJar and go back to using Kohsuke's FutureAdapter which would restore the old behavior. In any case, this only matters when the cache is cold; when the cache is warm, the behavior is identical to the old behavior (including the horrific existing behavior of reading the entire JAR file from disk for each .class file load operation, a great stress test of the OS page cache!).

All in all this change shuffles a few things around internally within the implementation but shouldn't result in any change in user-visible behavior, and in my opinion modernizes the codebase a little and improves readability, but if people feel the risk is too high, I could make this change somewhat more conservative as described above, or even close this PR entirely if people would rather not touch this messy code at all.

Testing done

Ran mvn clean verify, and ran a Pipeline job with the Git plugin and JGit with both warm and cold JAR caches, stepping through the relevant functionality in both cases in the debugger to verify correct operation before and after this change.

@basil basil added the internal label Jan 6, 2024
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

nice improvement

@basil basil merged commit b6a2c27 into jenkinsci:master Jan 8, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants