Skip to content

Commit

Permalink
[JENKINS-38867] Optimize performance of Actionable.getAllActions (#2582)
Browse files Browse the repository at this point in the history 
* Optimize Actionable.getAllActions.

* Also need to invalidate the cache when new plugins are installed.

* Various improvements to TransientActionFactory caching.
· Move the cache code to TransientActionFactory itself, for better encapsulation.
· Optimize getAction(Class) to not need to call getAllActions; avoids copying lists, and can avoid calling TransientActionFactory at all.
· Ensure that we maintain a separate cache per ExtensionList instance, so that static state is not leaked across Jenkins restarts.

* Updated TransientActionFactory to specify what kinds of actions it could produce.

* It turns out that changing type parameters for an extension, while generally binary-compatible, breaks reflective code in Jenkins and so this is not an option.
… hudson.ExtensionFinder$GuiceFinder$SezpozModule configure
WARNING: Failed to load com.cloudbees.hudson.plugins.folder.relocate.RelocationAction$TransientActionFactoryImpl
java.lang.LinkageError: Failed to resolve class com.cloudbees.hudson.plugins.folder.relocate.RelocationAction$TransientActionFactoryImpl
	at hudson.ExtensionFinder$GuiceFinder$SezpozModule.resolve(ExtensionFinder.java:489)
	at hudson.ExtensionFinder$GuiceFinder$SezpozModule.configure(ExtensionFinder.java:506)
	at …
	at hudson.ExtensionFinder$GuiceFinder.<init>(ExtensionFinder.java:280)
	at …
	at hudson.ClassicPluginStrategy.findComponents(ClassicPluginStrategy.java:472)
	at hudson.ExtensionList.load(ExtensionList.java:349)
	at hudson.ExtensionList.ensureLoaded(ExtensionList.java:287)
	at hudson.ExtensionList.getComponents(ExtensionList.java:167)
	at jenkins.model.Jenkins$8.onInitMilestoneAttained(Jenkins.java:1082)
	at jenkins.InitReactorRunner$1.onAttained(InitReactorRunner.java:82)
	at org.jvnet.hudson.reactor.ReactorListener$Aggregator.onAttained(ReactorListener.java:104)
	at org.jvnet.hudson.reactor.Reactor$1.run(Reactor.java:176)
	at org.jvnet.hudson.reactor.Reactor$Node.run(Reactor.java:117)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
	at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.reflect.MalformedParameterizedTypeException
	at sun.reflect.generics.reflectiveObjects.ParameterizedTypeImpl.validateConstructorArguments(ParameterizedTypeImpl.java:58)
	at sun.reflect.generics.reflectiveObjects.ParameterizedTypeImpl.<init>(ParameterizedTypeImpl.java:51)
	at sun.reflect.generics.reflectiveObjects.ParameterizedTypeImpl.make(ParameterizedTypeImpl.java:92)
	at sun.reflect.generics.factory.CoreReflectionFactory.makeParameterizedType(CoreReflectionFactory.java:105)
	at sun.reflect.generics.visitor.Reifier.visitClassTypeSignature(Reifier.java:140)
	at sun.reflect.generics.tree.ClassTypeSignature.accept(ClassTypeSignature.java:49)
	at sun.reflect.generics.repository.ClassRepository.getSuperclass(ClassRepository.java:90)
	at java.lang.Class.getGenericSuperclass(Class.java:777)
	at hudson.ExtensionFinder$GuiceFinder$SezpozModule.resolve(ExtensionFinder.java:470)
	... 29 more

* Remove actionType override to make for a simpler diff.

* Strengthened test to cover accesses to unrelated context types.

* Strengthening tests in a couple of ways.

* Javadoc improvements suggested by @oleg-nenashev.
  • Loading branch information
@jglick @oleg-nenashev
jglick authored and oleg-nenashev committed Nov 27, 2016
1 parent f50e0b5 commit 6360b96
Show file tree
Hide file tree
Showing 3 changed files with 210 additions and 19 deletions.
63 changes: 46 additions & 17 deletions core/src/main/java/hudson/model/Actionable.java
View file
Expand Up @@ -24,7 +24,6 @@
package hudson.model;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.ExtensionList;
import hudson.Util;
import java.util.ArrayList;
import java.util.Collection;
Expand All @@ -33,7 +32,6 @@
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.logging.Level;
import java.util.logging.Logger;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import jenkins.model.ModelObjectWithContextMenu;
Expand Down Expand Up @@ -68,13 +66,13 @@ public abstract class Actionable extends AbstractModelObject implements ModelObj
* This method by default returns only <em>persistent</em> actions
* (though some subclasses override it to return an extended unmodifiable list).
*
* @return
* may be empty but never null.
* @return a possibly empty list
* @deprecated Normally outside code should not call this method any more.
* Use {@link #getAllActions}, or {@link #addAction}, or {@link #replaceAction}.
* May still be called for compatibility reasons from subclasses predating {@link TransientActionFactory}.
*/
@Deprecated
@Nonnull
public List<Action> getActions() {
synchronized (this) {
if(actions == null) {
Expand All @@ -91,33 +89,53 @@ public List<Action> getActions() {
* @since 1.548
*/
@Exported(name="actions")
@Nonnull
public final List<? extends Action> getAllActions() {
List<Action> _actions = new ArrayList<Action>(getActions());
for (TransientActionFactory<?> taf : ExtensionList.lookup(TransientActionFactory.class)) {
if (taf.type().isInstance(this)) {
try {
_actions.addAll(createFor(taf));
} catch (Exception e) {
LOGGER.log(Level.SEVERE, "Could not load actions from " + taf + " for " + this, e);
List<Action> _actions = getActions();
boolean adding = false;
for (TransientActionFactory<?> taf : TransientActionFactory.factoriesFor(getClass(), Action.class)) {
Collection<? extends Action> additions = createFor(taf);
if (!additions.isEmpty()) {
if (!adding) { // need to make a copy
adding = true;
_actions = new ArrayList<>(_actions);
}
_actions.addAll(additions);
}
}
return Collections.unmodifiableList(_actions);
}

private <T> Collection<? extends Action> createFor(TransientActionFactory<T> taf) {
return taf.createFor(taf.type().cast(this));
try {
Collection<? extends Action> result = taf.createFor(taf.type().cast(this));
for (Action a : result) {
if (!taf.actionType().isInstance(a)) {
LOGGER.log(Level.WARNING, "Actions from {0} for {1} included {2} not assignable to {3}", new Object[] {taf, this, a, taf.actionType()});
return Collections.emptySet();
}
}
return result;
} catch (Exception e) {
LOGGER.log(Level.WARNING, "Could not load actions from " + taf + " for " + this, e);
return Collections.emptySet();
}
}

/**
* Gets all actions of a specified type that contributed to this object.
*
* @param type The type of action to return.
* @return
* may be empty but never null.
* @return an unmodifiable, possible empty list
* @see #getAction(Class)
*/
@Nonnull
public <T extends Action> List<T> getActions(Class<T> type) {
return Util.filter(getAllActions(), type);
List<T> _actions = Util.filter(getActions(), type);
for (TransientActionFactory<?> taf : TransientActionFactory.factoriesFor(getClass(), type)) {
_actions.addAll(Util.filter(createFor(taf), type));
}
return Collections.unmodifiableList(_actions);
}

/**
Expand Down Expand Up @@ -305,9 +323,20 @@ public Action getAction(int index) {
* @see #getActions(Class)
*/
public <T extends Action> T getAction(Class<T> type) {
for (Action a : getAllActions())
if (type.isInstance(a))
// Shortcut: if the persisted list has one, return it.
for (Action a : getActions()) {
if (type.isInstance(a)) {
return type.cast(a);
}
}
// Otherwise check transient factories.
for (TransientActionFactory<?> taf : TransientActionFactory.factoriesFor(getClass(), type)) {
for (Action a : createFor(taf)) {
if (type.isInstance(a)) {
return type.cast(a);
}
}
}
return null;
}

Expand Down
76 changes: 75 additions & 1 deletion core/src/main/java/jenkins/model/TransientActionFactory.java
View file
Expand Up @@ -24,12 +24,22 @@

package jenkins.model;

import com.google.common.cache.CacheBuilder;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import hudson.ExtensionList;
import hudson.ExtensionListListener;
import hudson.ExtensionPoint;
import hudson.model.Action;
import hudson.model.Actionable;
import hudson.model.TopLevelItem;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import javax.annotation.Nonnull;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

/**
* Allows you to add actions to any kind of object at once.
Expand All @@ -48,12 +58,76 @@ public abstract class TransientActionFactory<T> implements ExtensionPoint {
*/
public abstract Class<T> type();

/**
* A supertype of any actions this factory might produce.
* Defined so that factories which produce irrelevant actions need not be consulted by, e.g., {@link Actionable#getAction(Class)}.
* For historical reasons this defaults to {@link Action} itself.
* If your implementation was returning multiple disparate kinds of actions, it is best to split it into two factories.
* <p>If an API defines a abstract {@link Action} subtype and you are providing a concrete implementation,
* you may return the API type here to delay class loading.
* @return a bound for the result of {@link #createFor}
*/
public /* abstract */ Class<? extends Action> actionType() {
return Action.class;
}

/**
* Creates actions for a given object.
* This may be called frequently for the same object, so if your implementation is expensive, do your own caching.
* @param target an actionable object
* @return a possible empty set of actions
* @return a possible empty set of actions (typically either using {@link Collections#emptySet} or {@link Collections#singleton})
*/
public abstract @Nonnull Collection<? extends Action> createFor(@Nonnull T target);

/** @see <a href="http://stackoverflow.com/a/24336841/12916">no pairs/tuples in Java</a> */
private static class CacheKey {
private final Class<?> type;
private final Class<? extends Action> actionType;
CacheKey(Class<?> type, Class<? extends Action> actionType) {
this.type = type;
this.actionType = actionType;
}
@Override
public boolean equals(Object obj) {
return obj instanceof CacheKey && type == ((CacheKey) obj).type && actionType == ((CacheKey) obj).actionType;
}
@Override
public int hashCode() {
return type.hashCode() ^ actionType.hashCode();
}
}
@SuppressWarnings("rawtypes")
private static final LoadingCache<ExtensionList<TransientActionFactory>, LoadingCache<CacheKey, List<TransientActionFactory<?>>>> cache =
CacheBuilder.newBuilder().weakKeys().build(new CacheLoader<ExtensionList<TransientActionFactory>, LoadingCache<CacheKey, List<TransientActionFactory<?>>>>() {
@Override
public LoadingCache<CacheKey, List<TransientActionFactory<?>>> load(final ExtensionList<TransientActionFactory> allFactories) throws Exception {
final LoadingCache<CacheKey, List<TransientActionFactory<?>>> perJenkinsCache =
CacheBuilder.newBuilder().build(new CacheLoader<CacheKey, List<TransientActionFactory<?>>>() {
@Override
public List<TransientActionFactory<?>> load(CacheKey key) throws Exception {
List<TransientActionFactory<?>> factories = new ArrayList<>();
for (TransientActionFactory<?> taf : allFactories) {
Class<? extends Action> actionType = taf.actionType();
if (taf.type().isAssignableFrom(key.type) && (key.actionType.isAssignableFrom(actionType) || actionType.isAssignableFrom(key.actionType))) {
factories.add(taf);
}
}
return factories;
}
});
allFactories.addListener(new ExtensionListListener() {
@Override
public void onChange() {
perJenkinsCache.invalidateAll();
}
});
return perJenkinsCache;
}
});

@Restricted(NoExternalUse.class) // pending a need for it outside Actionable
public static Iterable<? extends TransientActionFactory<?>> factoriesFor(Class<?> type, Class<? extends Action> actionType) {
return cache.getUnchecked(ExtensionList.lookup(TransientActionFactory.class)).getUnchecked(new CacheKey(type, actionType));
}

}
90 changes: 89 additions & 1 deletion test/src/test/java/jenkins/model/TransientActionFactoryTest.java
View file
Expand Up @@ -24,14 +24,23 @@

package jenkins.model;

import hudson.Util;
import hudson.model.AbstractItem;
import hudson.model.AbstractProject;
import hudson.model.Action;
import hudson.model.FreeStyleProject;
import hudson.model.InvisibleAction;
import hudson.model.ProminentProjectAction;
import hudson.model.queue.FoldableAction;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import org.hamcrest.Matchers;
import static org.junit.Assert.*;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.MockFolder;
import org.jvnet.hudson.test.TestExtension;

public class TransientActionFactoryTest {
Expand All @@ -44,7 +53,8 @@ public class TransientActionFactoryTest {
}
@TestExtension("addedToAbstractItem") public static class TestItemFactory extends TransientActionFactory<AbstractItem> {
@Override public Class<AbstractItem> type() {return AbstractItem.class;}
@Override public Collection<? extends Action> createFor(AbstractItem i) {
@Override public Class<MyAction> actionType() {return MyAction.class;}
@Override public Collection<? extends MyAction> createFor(AbstractItem i) {
return Collections.singleton(new MyAction());
}
}
Expand All @@ -61,4 +71,82 @@ private static class MyAction implements Action {
}
}

@Test public void laziness() throws Exception {
FreeStyleProject p = r.createFreeStyleProject();
// testing getAction(Class)
assertNull(p.getAction(FoldableAction.class));
assertEquals(0, LazyFactory.count);
assertNotNull(p.getAction(ProminentProjectAction.class));
assertEquals(1, LazyFactory.count);
assertNotNull(p.getAction(MyProminentProjectAction.class));
assertEquals(2, LazyFactory.count);
LazyFactory.count = 0;
// getAllActions
List<? extends Action> allActions = p.getAllActions();
assertEquals(1, LazyFactory.count);
assertThat(Util.filter(allActions, FoldableAction.class), Matchers.<FoldableAction>iterableWithSize(0));
assertThat(Util.filter(allActions, ProminentProjectAction.class), Matchers.<ProminentProjectAction>iterableWithSize(1));
assertThat(Util.filter(allActions, MyProminentProjectAction.class), Matchers.<MyProminentProjectAction>iterableWithSize(1));
LazyFactory.count = 0;
// getActions(Class)
assertThat(p.getActions(FoldableAction.class), Matchers.<FoldableAction>iterableWithSize(0));
assertEquals(0, LazyFactory.count);
assertThat(p.getActions(ProminentProjectAction.class), Matchers.<ProminentProjectAction>iterableWithSize(1));
assertEquals(1, LazyFactory.count);
assertThat(p.getActions(MyProminentProjectAction.class), Matchers.<MyProminentProjectAction>iterableWithSize(1));
assertEquals(2, LazyFactory.count);
LazyFactory.count = 0;
// different context type
MockFolder d = r.createFolder("d");
assertNull(d.getAction(FoldableAction.class));
assertNull(d.getAction(ProminentProjectAction.class));
allActions = d.getAllActions();
assertThat(Util.filter(allActions, FoldableAction.class), Matchers.<FoldableAction>iterableWithSize(0));
assertThat(Util.filter(allActions, ProminentProjectAction.class), Matchers.<ProminentProjectAction>iterableWithSize(0));
assertThat(d.getActions(FoldableAction.class), Matchers.<FoldableAction>iterableWithSize(0));
assertThat(d.getActions(ProminentProjectAction.class), Matchers.<ProminentProjectAction>iterableWithSize(0));
assertEquals(0, LazyFactory.count);
}
@SuppressWarnings("rawtypes")
@TestExtension("laziness") public static class LazyFactory extends TransientActionFactory<AbstractProject> {
static int count;
@Override public Class<AbstractProject> type() {return AbstractProject.class;}
@Override public Class<? extends Action> actionType() {return ProminentProjectAction.class;}
@Override public Collection<? extends Action> createFor(AbstractProject p) {
count++;
return Collections.singleton(new MyProminentProjectAction());
}
}

@Test public void compatibility() throws Exception {
FreeStyleProject p = r.createFreeStyleProject();
// testing getAction(Class)
assertNull(p.getAction(FoldableAction.class));
assertEquals(1, OldFactory.count);
assertNotNull(p.getAction(ProminentProjectAction.class));
assertEquals(2, OldFactory.count);
OldFactory.count = 0;
// getAllActions
List<? extends Action> allActions = p.getAllActions();
assertEquals(1, OldFactory.count);
assertThat(Util.filter(allActions, FoldableAction.class), Matchers.<FoldableAction>iterableWithSize(0));
assertThat(Util.filter(allActions, ProminentProjectAction.class), Matchers.<ProminentProjectAction>iterableWithSize(1));
OldFactory.count = 0;
// getActions(Class)
assertThat(p.getActions(FoldableAction.class), Matchers.<FoldableAction>iterableWithSize(0));
assertEquals(1, OldFactory.count);
assertThat(p.getActions(ProminentProjectAction.class), Matchers.<ProminentProjectAction>iterableWithSize(1));
assertEquals(2, OldFactory.count);
}
@TestExtension("compatibility") public static class OldFactory extends TransientActionFactory<FreeStyleProject> {
static int count;
@Override public Class<FreeStyleProject> type() {return FreeStyleProject.class;}
@Override public Collection<? extends Action> createFor(FreeStyleProject p) {
count++;
return Collections.singleton(new MyProminentProjectAction());
}
}

private static class MyProminentProjectAction extends InvisibleAction implements ProminentProjectAction {}

}

0 comments on commit 6360b96

Please sign in to comment.