Looks like I deleted that in one of the revisions since.
On 22 April 2014 18:21, Ralph Goers <[email protected]> wrote: > You can remove the FIXME that was added. As we discussed in another email > the array is non-modifiable once it is created so increasing the size by > anything larger than the number of elements being added is a waste of > memory. > > Ralph > > On Apr 21, 2014, at 7: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(" ]"); > > > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > > -- Matt Sicker <[email protected]>
