When there was all the debate about having multiple parents, a lot of effort went into figuring out what the fastest implementation would be for the isInstanceOf methods as they have the potential to be called very frequently and in a multi-threaded way. As it stands now, the cases where 1 or 2 parents exist, there is no iteration that occurs. This should cover most cases. But if there are 3 or more parents, then the penalty of the newer for loop does take its toll. I don't think I ever recorded that difference because the new for loop was always slower, so why offer it as a solution when we were trying to be so performance conscious.
I suppose we just need to decide if readability is more important than speeding up a use case that will likely be hit less than 10% of the time. As for the other methods like add and remove, I see no problem with using the new for loop. On Mon, Apr 21, 2014 at 10:54 PM, Matt Sicker <[email protected]> wrote: > 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]> > -- Bruce Brouwer
