Optimizing ControllerState
alesj Oct 14, 2009 11:28 AMAt AS6 meeting we came across some "shocking" ControllerState numbers. :-)
Number of created instances was big,
and the number of equals() and hashCode() invocations was just outrageous ~ 12M.
(if I remember the numbers correctly)
The equals/hash invocations are result of state comparison,
which is done via List::indexOf. (we keep the states in a list)
protected int getStateIndex(ControllerState state, boolean allowNotFound) { if (state == null) throw new IllegalArgumentException("Null state"); int stateIndex = states.indexOf(state); if (stateIndex < 0 && allowNotFound == false) throw new IllegalArgumentException("No such state " + state + " in states " + states); return stateIndex; }
So, each time we compare two states, which we do quite a lot,
we're checking it against the list's elements.
This definitely calls for some optimization,
specially as we only expect to have a few diff ControllerState instances.
We should have some CS registry to reuse existing immutable instances,
not to mention changing compare impl.
This is my (very) naive approach, hence some feedback welcome.
Index: dependency/src/main/java/org/jboss/dependency/plugins/AbstractController.java =================================================================== --- dependency/src/main/java/org/jboss/dependency/plugins/AbstractController.java (revision 91575) +++ dependency/src/main/java/org/jboss/dependency/plugins/AbstractController.java (working copy) @@ -265,6 +265,8 @@ if (states.contains(state)) return; + state.order(before); + if (before == null) { states.add(state); @@ -1973,16 +1975,12 @@ public boolean isBeforeState(ControllerState state, ControllerState reference) { - int stateIndex = getStateIndex(state, true); - int referenceIndex = getStateIndex(reference, true); - return stateIndex < referenceIndex; + return state.compareTo(reference) < 0; } public boolean isAfterState(ControllerState state, ControllerState reference) { - int stateIndex = getStateIndex(state, true); - int referenceIndex = getStateIndex(reference, true); - return stateIndex > referenceIndex; + return state.compareTo(reference) > 0; } public Iterator<ControllerState> iterator() Index: dependency/src/main/java/org/jboss/dependency/spi/ControllerState.java =================================================================== --- dependency/src/main/java/org/jboss/dependency/spi/ControllerState.java (revision 91575) +++ dependency/src/main/java/org/jboss/dependency/spi/ControllerState.java (working copy) @@ -24,7 +24,9 @@ import java.io.ObjectStreamException; import java.io.Serializable; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.ArrayList; import org.jboss.util.JBossObject; import org.jboss.util.JBossStringBuilder; @@ -33,43 +35,48 @@ * Description of state. * * @author <a href="adrian@jboss.com">Adrian Brock</a> + * @author <a href="ales.justin@jboss.com">Ales Justin</a> * @version $Revision$ */ -public class ControllerState extends JBossObject implements Serializable +public class ControllerState extends JBossObject implements Serializable, Comparable<ControllerState> { - private static final long serialVersionUID = 2L; + private static final long serialVersionUID = 3L; /** Error */ - public static final ControllerState ERROR = new ControllerState("**ERROR**"); + public static final ControllerState ERROR = new ControllerState("**ERROR**", -1); /** Not installed state */ - public static final ControllerState NOT_INSTALLED = new ControllerState("Not Installed"); + public static final ControllerState NOT_INSTALLED = new ControllerState("Not Installed", 0); /** Pre install state */ - public static final ControllerState PRE_INSTALL = new ControllerState("PreInstall"); + public static final ControllerState PRE_INSTALL = new ControllerState("PreInstall", 1); /** Described state */ - public static final ControllerState DESCRIBED = new ControllerState("Described"); + public static final ControllerState DESCRIBED = new ControllerState("Described", 2); /** Instantiated state */ - public static final ControllerState INSTANTIATED = new ControllerState("Instantiated"); + public static final ControllerState INSTANTIATED = new ControllerState("Instantiated", 3); /** Configured state */ - public static final ControllerState CONFIGURED = new ControllerState("Configured"); + public static final ControllerState CONFIGURED = new ControllerState("Configured", 4); /** Create state */ - public static final ControllerState CREATE = new ControllerState("Create"); + public static final ControllerState CREATE = new ControllerState("Create", 5); /** Start state */ - public static final ControllerState START = new ControllerState("Start"); + public static final ControllerState START = new ControllerState("Start", 6); /** Installed state */ - public static final ControllerState INSTALLED = new ControllerState("Installed"); + public static final ControllerState INSTALLED = new ControllerState("Installed", 7); /** The state string */ protected final String stateString; + /** The dynamic order */ + protected int order; + private static Map<String, ControllerState> values = new HashMap<String, ControllerState>(); + private static List<ControllerState> states = new ArrayList<ControllerState>(); static { @@ -88,12 +95,15 @@ * Create a new state * * @param stateString the string representation + * @param order the order */ - public ControllerState(String stateString) + private ControllerState(String stateString, int order) { if (stateString == null) throw new IllegalArgumentException("Null state string"); + this.stateString = stateString; + this.order = order; } /** @@ -110,10 +120,16 @@ { if (object == null || object instanceof ControllerState == false) return false; + ControllerState other = (ControllerState) object; - return stateString.equals(other.stateString); + return order == other.order; } - + + public int compareTo(ControllerState other) + { + return order - other.order; + } + public void toString(JBossStringBuilder buffer) { buffer.append(stateString); @@ -133,4 +149,56 @@ { return values.get(stateString); } + + /** + * Get state instance. + * + * @param stateString the state string + * @return new state instance + */ + public static ControllerState getInstance(String stateString) + { + if (stateString == null) + throw new IllegalArgumentException("Null state string."); + + ControllerState state = values.get(stateString); + if (state != null) + return state; + + state = new ControllerState(stateString, -1); + // cache it + values.put(stateString, state); + return state; + } + + /** + * Modify order. + * + * @param before the before ref + */ + public void order(ControllerState before) + { + // do string, as we might not be properly ordered yet + for (ControllerState cs : states) + { + if (cs.stateString.equals(stateString)) + return; + } + + if (before == null) + { + order = states.size(); + states.add(this); + } + else + { + int index = states.indexOf(before); + order = index; + // shift others + for (int i = index; i < states.size(); i++) + states.get(i).order++; + // add this state + states.add(index, this); + } + } }