You are viewing a plain text version of this content. The canonical link for it is here.
Posted to log4j-dev@logging.apache.org by Gary Gregory <ga...@gmail.com> on 2014/04/22 04:29:33 UTC
Re: svn commit: r1589014 - /logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
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, <ma...@apache.org> 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: garydgregory@gmail.com | ggregory@apache.org
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
Re: svn commit: r1589014 - /logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
Posted by Matt Sicker <bo...@gmail.com>.
My guess is that javac creates a wrapper Iterator for arrays which seems
extraneous. Really, the for-each loop could be implemented far better than
only support Iterable. It could have supported Iterator, Enumerator, and
the five other iterator-like things in the standard JDK. At the very least,
it should implement a foreach loop on arrays via indices.
On 21 April 2014 21:54, Ralph Goers <rg...@apache.org> wrote:
> 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 <br...@gmail.com>
> 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 <bo...@gmail.com> 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 <ga...@gmail.com> 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, <ma...@apache.org> 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: garydgregory@gmail.com | ggregory@apache.org
>>> 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 <bo...@gmail.com>
>>
>
>
>
> --
>
> Bruce Brouwer
>
>
--
Matt Sicker <bo...@gmail.com>
Re: svn commit: r1589014 - /logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
Posted by Ralph Goers <rg...@apache.org>.
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 <br...@gmail.com> 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 <bo...@gmail.com> 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 <ga...@gmail.com> 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, <ma...@apache.org> 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: garydgregory@gmail.com | ggregory@apache.org
>>> 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 <bo...@gmail.com>
>
>
>
> --
>
> Bruce Brouwer
Re: svn commit: r1589014 - /logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
Posted by Bruce Brouwer <br...@gmail.com>.
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 <bo...@gmail.com> 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 <ga...@gmail.com> 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, <ma...@apache.org> 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: garydgregory@gmail.com | ggregory@apache.org
>> 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 <bo...@gmail.com>
>
--
Bruce Brouwer
Re: svn commit: r1589014 - /logging/log4j/log4j2/trunk/log4j-api/src/main/java/org/apache/logging/log4j/MarkerManager.java
Posted by Matt Sicker <bo...@gmail.com>.
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 <ga...@gmail.com> 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, <ma...@apache.org> 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: garydgregory@gmail.com | ggregory@apache.org
> 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 <bo...@gmail.com>