I dunno, Bruce said he benchmarked it. I figured the Java compiler wouldn't use iterators in a for each loop on an array, but I don't know what the rules are for that.
On 21 April 2014 20:29, Gary Gregory <[email protected]> wrote: > Ugh, almost -1, this micro-optimization and move away from for-each loops > is painful. The for-each construct is easy to read and maintain. > > What is the actual performance measurement that justifies this changes? > > Are call sites expected to call the maker manager in some super tight loop? > > Gary > > > On Mon, Apr 21, 2014 at 10:17 PM, <[email protected]> wrote: > >> Author: mattsicker >> Date: Tue Apr 22 02:17:32 2014 >> New Revision: 1589014 >> >> URL: http://svn.apache.org/r1589014 >> Log: >> Performance enhancements. >> >> - Replaced for each loops with optimised for loops. >> - Added noinspection comments to said loops so I don't change them back >> accidentally later on. >> - Used final where possible. >> - Made some private methods static. >> - There's some duplicate code in this file I noticed, but I'm not going >> to refactor it just yet. Are there performance benefits or something? >> - Rearranged method parameters in one method to allow for varargs >> (style). >> >> Modified: >> >> logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java >> >> Modified: >> logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java >> URL: >> http://svn.apache.org/viewvc/logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java?rev=1589014&r1=1589013&r2=1589014&view=diff >> >> ============================================================================== >> --- >> logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java >> (original) >> +++ >> logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java >> Tue Apr 22 02:17:32 2014 >> @@ -27,7 +27,7 @@ import java.util.concurrent.ConcurrentMa >> */ >> public final class MarkerManager { >> >> - private static ConcurrentMap<String, Marker> markerMap = new >> ConcurrentHashMap<String, Marker>(); >> + private static final ConcurrentMap<String, Marker> markerMap = new >> ConcurrentHashMap<String, Marker>(); >> >> private MarkerManager() { >> } >> @@ -89,19 +89,20 @@ public final class MarkerManager { >> } >> >> @Override >> - public synchronized Marker add(Marker parent) { >> + public synchronized Marker add(final Marker parent) { >> if (parent == null) { >> throw new IllegalArgumentException("A parent marker must >> be specified"); >> } >> // It is not strictly necessary to copy the variable here >> but it should perform better than >> // Accessing a volatile variable multiple times. >> - Marker[] localParents = this.parents; >> + final Marker[] localParents = this.parents; >> // Don't add a parent that is already in the hierarchy. >> if (localParents != null && (contains(parent, localParents) >> || parent.isInstanceOf(this))) { >> return this; >> } >> - int size = localParents == null ? 1 : localParents.length + >> 1; >> - Marker[] markers = new Marker[size]; >> + // FIXME: should we really be only increasing the array size >> by 1 each time? why not something like log(n)? >> + final int size = localParents == null ? 1 : >> localParents.length + 1; >> + final Marker[] markers = new Marker[size]; >> if (localParents != null) { >> // It's perfectly OK to call arraycopy in a synchronized >> context; it's still faster >> //noinspection CallToNativeMethodWhileLocked >> @@ -113,15 +114,16 @@ public final class MarkerManager { >> } >> >> @Override >> - public synchronized boolean remove(Marker parent) { >> + public synchronized boolean remove(final Marker parent) { >> if (parent == null) { >> throw new IllegalArgumentException("A parent marker must >> be specified"); >> } >> - Marker[] localParents = this.parents; >> + final Marker[] localParents = this.parents; >> if (localParents == null) { >> return false; >> } >> - if (localParents.length == 1) { >> + final int localParentsLength = localParents.length; >> + if (localParentsLength == 1) { >> if (localParents[0].equals(parent)) { >> parents = null; >> return true; >> @@ -129,10 +131,12 @@ public final class MarkerManager { >> return false; >> } >> int index = 0; >> - Marker[] markers = new Marker[localParents.length - 1]; >> - for (Marker marker : localParents) { >> + final Marker[] markers = new Marker[localParentsLength - 1]; >> + //noinspection ForLoopReplaceableByForEach >> + for (int i = 0; i < localParentsLength; i++) { >> + final Marker marker = localParents[i]; >> if (!marker.equals(parent)) { >> - if (index == localParents.length - 1) { >> + if (index == localParentsLength - 1) { >> return false; >> } >> markers[index++] = marker; >> @@ -143,11 +147,11 @@ public final class MarkerManager { >> } >> >> @Override >> - public Marker setParents(Marker... markers) { >> + public Marker setParents(final Marker... markers) { >> if (markers == null || markers.length == 0) { >> this.parents = null; >> } else { >> - Marker[] array = new Marker[markers.length]; >> + final Marker[] array = new Marker[markers.length]; >> System.arraycopy(markers, 0, array, 0, markers.length); >> this.parents = array; >> } >> @@ -185,16 +189,19 @@ public final class MarkerManager { >> if (this == marker) { >> return true; >> } >> - Marker[] localParents = parents; >> + final Marker[] localParents = parents; >> if (localParents != null) { >> // With only one or two parents the for loop is slower. >> - if (localParents.length == 1) { >> + final int localParentsLength = localParents.length; >> + if (localParentsLength == 1) { >> return checkParent(localParents[0], marker); >> } >> - if (localParents.length == 2) { >> + if (localParentsLength == 2) { >> return checkParent(localParents[0], marker) || >> checkParent(localParents[1], marker); >> } >> - for (final Marker localParent : localParents) { >> + //noinspection ForLoopReplaceableByForEach >> + for (int i = 0; i < localParentsLength; i++) { >> + final Marker localParent = localParents[i]; >> if (checkParent(localParent, marker)) { >> return true; >> } >> @@ -206,25 +213,30 @@ public final class MarkerManager { >> @Override >> public boolean isInstanceOf(final String markerName) { >> if (markerName == null) { >> + // FIXME: and normally you'd throw an NPE here (ditto >> for other cases above) >> throw new IllegalArgumentException("A marker name is >> required"); >> } >> if (markerName.equals(this.getName())) { >> return true; >> } >> // Use a real marker for child comparisons. It is faster >> than comparing the names. >> - Marker marker = markerMap.get(markerName); >> + final Marker marker = markerMap.get(markerName); >> if (marker == null) { >> + // FIXME: shouldn't this just return false? >> throw new IllegalArgumentException("No marker exists >> with the name " + markerName); >> } >> - Marker[] localParents = parents; >> + final Marker[] localParents = parents; >> if (localParents != null) { >> - if (localParents.length == 1) { >> + final int localParentsLength = localParents.length; >> + if (localParentsLength == 1) { >> return checkParent(localParents[0], marker); >> } >> - if (localParents.length == 2) { >> + if (localParentsLength == 2) { >> return checkParent(localParents[0], marker) || >> checkParent(localParents[1], marker); >> } >> - for (final Marker localParent : localParents) { >> + //noinspection ForLoopReplaceableByForEach >> + for (int i = 0; i < localParentsLength; i++) { >> + final Marker localParent = localParents[i]; >> if (checkParent(localParent, marker)) { >> return true; >> } >> @@ -234,19 +246,22 @@ public final class MarkerManager { >> return false; >> } >> >> - private boolean checkParent(Marker parent, Marker marker) { >> + private static boolean checkParent(final Marker parent, final >> Marker marker) { >> if (parent == marker) { >> return true; >> } >> - Marker[] localParents = parent instanceof Log4jMarker ? >> ((Log4jMarker)parent).parents : parent.getParents(); >> + final Marker[] localParents = parent instanceof Log4jMarker >> ? ((Log4jMarker)parent).parents : parent.getParents(); >> if (localParents != null) { >> - if (localParents.length == 1) { >> + final int localParentsLength = localParents.length; >> + if (localParentsLength == 1) { >> return checkParent(localParents[0], marker); >> } >> - if (localParents.length == 2) { >> + if (localParentsLength == 2) { >> return checkParent(localParents[0], marker) || >> checkParent(localParents[1], marker); >> } >> - for (final Marker localParent : localParents) { >> + //noinspection ForLoopReplaceableByForEach >> + for (int i = 0; i < localParentsLength; i++) { >> + final Marker localParent = localParents[i]; >> if (checkParent(localParent, marker)) { >> return true; >> } >> @@ -258,9 +273,10 @@ public final class MarkerManager { >> /* >> * Called from add while synchronized. >> */ >> - private boolean contains(Marker parent, Marker... localParents) { >> - >> - for (Marker marker : localParents) { >> + private static boolean contains(final Marker parent, final >> Marker... localParents) { >> + //noinspection ForLoopReplaceableByForEach >> + for (int i = 0, localParentsLength = localParents.length; i >> < localParentsLength; i++) { >> + final Marker marker = localParents[i]; >> if (marker == parent) { >> return true; >> } >> @@ -293,26 +309,29 @@ public final class MarkerManager { >> >> @Override >> public String toString() { >> + // FIXME: might want to use an initial capacity; the default >> is 16 (or str.length() + 16) >> final StringBuilder sb = new StringBuilder(name); >> - Marker[] localParents = parents; >> + final Marker[] localParents = parents; >> if (localParents != null) { >> - addParentInfo(localParents, sb); >> + addParentInfo(sb, localParents); >> } >> return sb.toString(); >> } >> >> - private void addParentInfo(Marker[] parents, StringBuilder sb) { >> + private static void addParentInfo(final StringBuilder sb, final >> Marker... parents) { >> sb.append("[ "); >> boolean first = true; >> - for (Marker marker : parents) { >> + //noinspection ForLoopReplaceableByForEach >> + for (int i = 0, parentsLength = parents.length; i < >> parentsLength; i++) { >> + final Marker marker = parents[i]; >> if (!first) { >> sb.append(", "); >> } >> first = false; >> sb.append(marker.getName()); >> - Marker[] p = marker instanceof Log4jMarker ? >> ((Log4jMarker)marker).parents : marker.getParents(); >> + final Marker[] p = marker instanceof Log4jMarker ? >> ((Log4jMarker) marker).parents : marker.getParents(); >> if (p != null) { >> - addParentInfo(p, sb); >> + addParentInfo(sb, p); >> } >> } >> sb.append(" ]"); >> >> >> > > > -- > E-Mail: [email protected] | [email protected] > Java Persistence with Hibernate, Second > Edition<http://www.manning.com/bauer3/> > JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> > Spring Batch in Action <http://www.manning.com/templier/> > Blog: http://garygregory.wordpress.com > Home: http://garygregory.com/ > Tweet! http://twitter.com/GaryGregory > -- Matt Sicker <[email protected]>
