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.

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

Jump to bottom

[JENKINS-38867] Optimize Actionable.getAllActions #2582

Merged
merged 13 commits into from
Nov 27, 2016
Merged

[JENKINS-38867] Optimize Actionable.getAllActions #2582

Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f888361
Optimize Actionable.getAllActions.
jglick Oct 6, 2016
d26c742
Also need to invalidate the cache when new plugins are installed.
jglick Oct 6, 2016
3679ee9
Merge branch 'master' into TransientActionFactory-opt
jglick Oct 10, 2016
ae1ed2c
Various improvements to TransientActionFactory caching.
jglick Oct 10, 2016
1ba0ef8
Merge branch 'master' into TransientActionFactory-opt
jglick Oct 13, 2016
4d2bc22
Updated TransientActionFactory to specify what kinds of actions it co…
jglick Oct 13, 2016
8a99648
It turns out that changing type parameters for an extension, while ge…
jglick Oct 13, 2016
69ff255
Remove actionType override to make for a simpler diff.
jglick Oct 13, 2016
b514321
Strengthened test to cover accesses to unrelated context types.
jglick Oct 13, 2016
76a1b75
Strengthening tests in a couple of ways.
jglick Oct 13, 2016
dfdcdc8
Merge branch 'master' into TransientActionFactory-opt
jglick Nov 6, 2016
5c2c70b
Javadoc improvements suggested by @oleg-nenashev.
jglick Nov 6, 2016
b83b7aa
Merge branch 'master' into TransientActionFactory-opt
jglick Nov 8, 2016
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
51 changes: 44 additions & 7 deletions core/src/main/java/hudson/model/Actionable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,11 @@
*/
package hudson.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.Util;
import java.util.ArrayList;
import java.util.Collection;
Expand Down Expand Up @@ -91,18 +95,51 @@ public List<Action> getActions() {
*/
@Exported(name="actions")
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;
synchronized (Actionable.class) {
Copy link
Member

@svanoort svanoort Oct 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

🐛 ❗️ 💥 We're synchronizing every single getAllActions call on this single class. Instant lock contention all over the place.

Synchronize on this.getClass() I think. If we can avoid synchronization at all we should, though.

Edit: unless my coffee is still kicking in and I've misunderstood -- intent is to synchronize on this specific class, not the overall actionable if at all possible though, and better neither.

Copy link
Member

Choose a reason for hiding this comment

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

Actually @svanoort you are reading this wrong. factoryClass is a lazy singleton cache. Would be better to leverage the lazy instantiation pattern...

private static final class ResourceHolder {
  static final LoadingCache<Class<? extends Actionable>, Collection<? extends TransientActionFactory<?>>> factoryCache;
  static {
                @SuppressWarnings("rawtypes")
                final ExtensionList<TransientActionFactory> allFactories = ExtensionList.lookup(TransientActionFactory.class);
                factoryCache = CacheBuilder.newBuilder().build(new CacheLoader<Class<? extends Actionable>, Collection<? extends TransientActionFactory<?>>>() {
                    @Override
                    public Collection<? extends TransientActionFactory<?>> load(Class<? extends Actionable> implType) throws Exception {
                        List<TransientActionFactory<?>> factories = new ArrayList<>();
                        for (TransientActionFactory<?> taf : allFactories) {
                            if (taf.type().isAssignableFrom(implType)) {
                                factories.add(taf);
                            }
                        }
                        return factories;
                    }
                });
                allFactories.addListener(new ExtensionListListener() {
                    @Override
                    public void onChange() {
                        factoryCache.invalidateAll();
                    }
                });
  }
}

And then here we just go ResourceHolder.factoryCache without care and the JVM can optimize better.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, so more coffee it is then. Agree totally that the ResourceHolder approach is far better and will improve performance.

Copy link
Member Author

Choose a reason for hiding this comment

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

I doubt that it matters, since after the cache is initialized during startup the code will just be doing a null check and the JVM is good at optimizing away contention on monitors in simple cases, but if it makes you happier I can switch to a resource holder pattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually that is silly. Any kind of cache lookup needs to acquire a lock anyway, so we might as well use just one.

Copy link
Member Author

Choose a reason for hiding this comment

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

However, there is a more subtle issue with restarting Jenkins which I will work to fix.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, maybe not - explicit contention for something that will be invoked often by numerous threads raises a flag for me on general principles. I agree it shouldn't be as expensive as initially I thought when skimming through (and hopefully won't be a problem), but still worth a 🐜 on general principles.

Any kind of cache lookup needs to acquire a lock anyway, so we might as well use just one

IIRC most of the hashmaps and concurrent maps are based on locking per-slot and not on the whole, so contention is extremely rare. Actions are likely to get requested by many threads, and frequently, so contention will be high, even if very brief.

if (factoryCache == null) {
@SuppressWarnings("rawtypes")
final ExtensionList<TransientActionFactory> allFactories = ExtensionList.lookup(TransientActionFactory.class);
factoryCache = CacheBuilder.newBuilder().build(new CacheLoader<Class<? extends Actionable>, Collection<? extends TransientActionFactory<?>>>() {
@Override
public Collection<? extends TransientActionFactory<?>> load(Class<? extends Actionable> implType) throws Exception {
List<TransientActionFactory<?>> factories = new ArrayList<>();
for (TransientActionFactory<?> taf : allFactories) {
if (taf.type().isAssignableFrom(implType)) {
factories.add(taf);
}
}
return factories;
}
});
allFactories.addListener(new ExtensionListListener() {
@Override
public void onChange() {
factoryCache.invalidateAll();
Copy link
Member

Choose a reason for hiding this comment

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

How often will this get called?

Copy link
Member Author

Choose a reason for hiding this comment

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

After startup it should only get called if you dynamically install a plugin, which is rare.

Copy link
Member

Choose a reason for hiding this comment

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

Fair. I can see other places this strategy might be useful, so may borrow it in the future.

}
});
}
}
for (TransientActionFactory<?> taf : factoryCache.getUnchecked(getClass())) {
Collection<? extends Action> additions;
try {
additions = createFor(taf);
} catch (Exception e) {
LOGGER.log(Level.SEVERE, "Could not load actions from " + taf + " for " + this, e);
continue;
}
if (!additions.isEmpty()) {
if (!adding) { // need to make a copy
adding = true;
_actions = new ArrayList<>(_actions);
}
_actions.addAll(additions);
}
}
return Collections.unmodifiableList(_actions);
}
private static LoadingCache<Class<? extends Actionable>, Collection<? extends TransientActionFactory<?>>> factoryCache;
private <T> Collection<? extends Action> createFor(TransientActionFactory<T> taf) {
return taf.createFor(taf.type().cast(this));
}
Expand Down