I benchmarked it but didn't save the results. The difference was quite noticeable - in the neighborhood of 10% as I recall. I have no idea why an old style for loop is "painful" - it is quite clear what it is doing.
Ralph > On Apr 21, 2014, at 8:22 PM, Bruce Brouwer <[email protected]> wrote: > > 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 >>> JUnit in Action, Second Edition >>> Spring Batch in Action >>> Blog: http://garygregory.wordpress.com >>> Home: http://garygregory.com/ >>> Tweet! http://twitter.com/GaryGregory >> >> >> >> -- >> Matt Sicker <[email protected]> > > > > -- > > Bruce Brouwer
