Skip to content

Replacing Guava’s CacheBuilder with Java Platform equivalents #220

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.

Sign up for GitHub

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

Merged
jglick merged 2 commits into from
May 8, 2021
Merged

Replacing Guava’s CacheBuilder with Java Platform equivalents #220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1dd8b1e
Replacing Guava’s `CacheBuilder` with Java Platform equivalents
jglick May 7, 2021
3338be1
`URL`-as-map-key bug was actually present before, but SpotBugs did no…
jglick May 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions core/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@
<version>1.8.3</version>
<optional>true</optional>
</dependency>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>11.0.1</version>
</dependency>
<dependency>
<groupId>org.kohsuke</groupId>
<artifactId>asm5</artifactId>
Expand Down
38 changes: 25 additions & 13 deletions core/src/main/java/org/kohsuke/stapler/AbstractTearOff.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,16 @@

package org.kohsuke.stapler;

import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

import java.io.File;
import java.lang.ref.Reference;
import java.lang.ref.SoftReference;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URL;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.regex.Matcher;
Expand All @@ -52,13 +54,13 @@ public abstract class AbstractTearOff<CLT,S,E extends Exception> extends Caching

private static final class ExpirableCacheHit<S> {
private final long timestamp;
private final S script;
private final Reference<S> script;
ExpirableCacheHit(long timestamp, S script) {
this.timestamp = timestamp;
this.script = script;
this.script = new SoftReference<>(script);
}
}
private final Cache<URL, ExpirableCacheHit<S>> cachedScripts = CacheBuilder.newBuilder().softValues().build();
private final Map<String, ExpirableCacheHit<S>> cachedScripts = new ConcurrentHashMap<>();

protected AbstractTearOff(MetaClass owner, Class<CLT> cltClass) {
this.owner = owner;
Expand Down Expand Up @@ -122,7 +124,7 @@ public S resolveScript(String name) throws E {
LOGGER.log(Level.FINE, "no timestamp associated with {0}", file);
return parseScript(res);
} else {
ExpirableCacheHit<S> cached = cachedScripts.getIfPresent(res);
ExpirableCacheHit<S> cached = cachedScripts.get(res.toString());
if (cached == null) {
S script;
if (LOGGER.isLoggable(Level.FINE)) {
Expand All @@ -136,15 +138,25 @@ public S resolveScript(String name) throws E {
LOGGER.log(Level.FINE, "cache miss on {0}", res);
script = parseScript(res);
}
cachedScripts.put(res, new ExpirableCacheHit<>(timestamp, script));
cachedScripts.put(res.toString(), new ExpirableCacheHit<>(timestamp, script));
return script;
} else if (timestamp == cached.timestamp) {
LOGGER.log(Level.FINE, "cache hit on {0}", res);
return cached.script;
} else {
LOGGER.log(Level.FINE, "expired cache hit on {0}", res);
S script = parseScript(res);
cachedScripts.put(res, new ExpirableCacheHit<>(timestamp, script));
S script;
if (timestamp == cached.timestamp) {
script = cached.script.get();
if (script == null) {
LOGGER.log(Level.FINE, "cache hit on {0} but value collected", res);
} else {
LOGGER.log(Level.FINE, "cache hit on {0}", res);
}
} else {
LOGGER.log(Level.FINE, "expired cache hit on {0}", res);
script = null;
}
if (script == null) {
script = parseScript(res);
cachedScripts.put(res.toString(), new ExpirableCacheHit<>(timestamp, script));
}
return script;
}
}
Expand Down
47 changes: 25 additions & 22 deletions core/src/main/java/org/kohsuke/stapler/CachingScriptLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
package org.kohsuke.stapler;

import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;

import java.lang.ref.Reference;
import java.lang.ref.SoftReference;
import java.net.URL;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;

/**
* Convenient base class for caching loaded scripts.
Expand All @@ -22,20 +23,8 @@ public abstract class CachingScriptLoader<S, E extends Exception> {
*
* So it's important to allow Scripts to be garbage collected.
* This is not an ideal fix, but it works.
*
* {@link Optional} is used as Google Collection doesn't allow null values in a map.
*/
private final LoadingCache<String,Optional<S>> scripts = CacheBuilder.newBuilder().softValues().build(new CacheLoader<String, Optional<S>>() {
public Optional<S> load(String from) {
try {
return Optional.create(loadScript(from));
} catch (RuntimeException e) {
throw e; // pass through
} catch (Exception e) {
throw new ScriptLoadException(e);
}
}
});
private final Map<String, Optional<Reference<S>>> scripts = new ConcurrentHashMap<>();

/**
* Locates the view script of the given name.
Expand All @@ -56,10 +45,24 @@ public Optional<S> load(String from) {
* @return null if none was found.
*/
public S findScript(String name) throws E {
if (MetaClass.NO_CACHE)
if (MetaClass.NO_CACHE) {
return loadScript(name);
else
return scripts.getUnchecked(name).get();
} else {
Optional<Reference<S>> sr = scripts.get(name);
S s;
if (sr == null) { // never before computed
s = null;
} else if (!sr.isPresent()) { // cached as null
return null;
} else { // cached as non-null; may or may not still have value
s = sr.get().get();
}
if (s == null) { // needs to be computed
s = loadScript(name);
scripts.put(name, s == null ? Optional.empty() : Optional.of(new SoftReference<>(s)));
}
return s;
}
}

/**
Expand All @@ -70,8 +73,8 @@ public S findScript(String name) throws E {
/**
* Discards the cached script.
*/
public synchronized void clearScripts() {
scripts.invalidateAll();
public void clearScripts() {
scripts.clear();
}

protected abstract URL getResource(String name);
Expand Down
15 changes: 6 additions & 9 deletions core/src/main/java/org/kohsuke/stapler/Function.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,6 @@

package org.kohsuke.stapler;

import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import org.apache.commons.lang.StringUtils;
import org.kohsuke.stapler.interceptor.Interceptor;
import org.kohsuke.stapler.interceptor.InterceptorAnnotation;
Expand All @@ -38,7 +35,6 @@
import java.lang.invoke.MethodHandle;
import java.lang.invoke.WrongMethodTypeException;
import java.lang.reflect.AnnotatedElement;
import java.lang.reflect.Executable;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
Expand Down Expand Up @@ -195,7 +191,7 @@ Object bindAndInvoke(Object o, StaplerRequest req, StaplerResponse rsp, Object..
}

// if the databinding method is provided, call that
Function binder = PARSE_METHODS.getUnchecked(t);
Function binder = PARSE_METHODS.get(t);
if (binder!=RETURN_NULL) {
arguments[i] = binder.bindAndInvoke(null,req,rsp);
continue;
Expand All @@ -216,7 +212,7 @@ Object bindAndInvoke(Object o, StaplerRequest req, StaplerResponse rsp, Object..
* Computing map that discovers the static 'fromStapler' method from a class.
* The discovered method will be returned as a Function so that the invocation can do parameter injections.
*/
private static final LoadingCache<Class,Function> PARSE_METHODS;
private static final ClassValue<Function> PARSE_METHODS;
private static final Function RETURN_NULL;

static {
Expand All @@ -226,8 +222,9 @@ Object bindAndInvoke(Object o, StaplerRequest req, StaplerResponse rsp, Object..
throw new AssertionError(e); // impossible
}

PARSE_METHODS = CacheBuilder.newBuilder().weakKeys().build(new CacheLoader<Class,Function>() {
public Function load(Class from) {
PARSE_METHODS = new ClassValue<Function>() {
@Override
public Function computeValue(Class<?> from) {
Copy link
Member Author

@jglick jglick May 7, 2021

Choose a reason for hiding this comment

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

Not only simpler, but should fix a memory leak in the rare case that fromStapler is actually defined: #213 (comment)

// MethdFunction for invoking a static method as a static method
FunctionList methods = new ClassDescriptor(from).methods.name("fromStapler");
switch (methods.size()) {
Expand Down Expand Up @@ -259,7 +256,7 @@ public Object invoke(StaplerRequest req, StaplerResponse rsp, Object o, Object..
};
}
}
});
};
}

/**
Expand Down
51 changes: 0 additions & 51 deletions core/src/main/java/org/kohsuke/stapler/Optional.java
View file
Open in desktop

This file was deleted.

5 changes: 2 additions & 3 deletions core/src/main/java/org/kohsuke/stapler/ScriptLoadException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,9 @@
package org.kohsuke.stapler;

/**
* Indicates a failure to load a script.
*
* @author Kohsuke Kawaguchi
* @deprecated No longer used.
*/
@Deprecated
public class ScriptLoadException extends RuntimeException {
public ScriptLoadException(String message, Throwable cause) {
super(message, cause);
Expand Down