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 Bruce Brouwer <br...@gmail.com> on 2014/04/22 02:46:02 UTC
Some things I noticed with the Marker implementation
I saw that some small changes were being made to the Markers. I had a few
thoughts regarding them:
1) Use of array iterator instead of indexed for loop.
for (Marker marker : localParents)
instead of
for (int i = 0; i < localParents.length; i++)
When I was doing my performance benchmarks, I was finding the latter to be
faster. I'm guessing this is simply because a new Iterable object needs to
be created to iterate over the array.
For most methods, such as add, remove, this was not a big deal. But for the
isInstanceOf and checkParent methods, we want those to be as fast as
possible.
2) isInstanceOf(String markerName)
Instead of throwing an IllegalArgumentException when a marker of name
markerName doesn't exist, why don't we simply return false? I don't want an
IllegalArgumentException to happen because I'm testing a markerName.
--
Bruce Brouwer
Re: Some things I noticed with the Marker implementation
Posted by Bruce Brouwer <br...@gmail.com>.
The only reason to have more than the single vararg version is for
performance. I don't think the marker .add method is going to be called
often so ultra-performance is not necessary. I say don't bother with the
one or two param variant. I just want the varargs version.
On Tue, Apr 22, 2014 at 8:30 PM, Matt Sicker <bo...@gmail.com> wrote:
>
>
> On Tuesday, 22 April 2014, Bruce Brouwer <br...@gmail.com> wrote:
>
>> I don't think performance is a real concern on this method. Style is what
>> I am concerned about. My vote would be to change the add method to accept
>> varargs.
>>
> We could include both. Or have one, two, and variable versions.
>
>
>> On Apr 22, 2014 11:45 AM, "Ralph Goers" <ra...@dslextreme.com>
>> wrote:
>>
>> I thought about the add method with multiple arguments but thought that
>> it sort of goes against the builder pattern. However, I am sure it would
>> perform better.
>>
>> Ralph
>>
>> On Apr 22, 2014, at 5:18 AM, Bruce Brouwer <br...@gmail.com>
>> wrote:
>>
>> Don't worry about the part of my comment that you were confused about. I
>> see now that varargs are not slower than array parameters, so long as you
>> pass in an array.
>>
>> 1) I did find some other things to talk about. This comment:
>> // It is not strictly necessary to copy the variable here but
>> it should perform better than
>> // Accessing a volatile variable multiple times.
>> I believe should be removed. This method is not synchronized so iterating
>> over an array of 5 elements while another thread removes an element would
>> cause the array to shrink to 4 and cause a possible ArrayIndexOutOfBounds
>> exception. I believe it is absolutely necessary to make a copy of the array.
>>
>> 2) Something I missed earlier regarding add that your FIXME reminded me
>> of. I would like to see the add method take varargs so that I can add more
>> than one parent at a time. I know the fluent nature means I could just call
>> marker.add(name1).add(name2), but why not marker.add(name1, name2)?
>>
>> 3) public boolean isInstanceOf(final String markerName) checks for a null
>> MarkerName and throws IllegalArgumentException, but the current
>> implementation allows a null marker name. I believe that check should be
>> removed and code it so nulls can be handled in the method. Either that or
>> make the other methods throw exceptions if you pass in a null marker name.
>> It is possible that adding null checks will slow this method down so I
>> won't be surprised if we change other methods to disallow null marker names.
>>
>> 4) Please make the FIXME in isInstanceOf happen that returns false
>> instead of throwing IllegalArgumentException when the marker does not exist.
>>
>>
>>
>> On Tue, Apr 22, 2014 at 12:18 AM, Matt Sicker <bo...@gmail.com> wrote:
>>
>> Actually, now I'm confused about that. Bruce, could you take a look at
>> the current trunk version of MarkerManager?
>>
>>
>> On 21 April 2014 21:48, Ralph Goers <rg...@apache.org> wrote:
>>
>> I'm not sure what you are referring to - the parameters? Using a copy of
>> a volatile reference is usually faster than using the volatile reference
>> itself. I think that is what Bruce is referring to.
>>
>> Ralph
>>
>> On Apr 21, 2014, at 7:20 PM, Matt Sicker <bo...@gmail.com> wrote:
>>
>> Using Marker... or Marker[] compile to the same bytecode. It should only
>> affect compile-time stuff unless I'm mistaken.
>>
>>
>> On 21 April 2014 20:17, Bruce Brouwer <br...@gmail.com> wrote:
>>
>> Yes, I did. The arrays are faster in 90% of cases. The only time I got
>> the HashSet to be faster was when I was caching the entire marker hierarchy
>> and the hierarchy was more than 3 levels deep. That is definitely not the
>> most common case.
>>
>> Also, I think the Marker... localParents parameter might be more
>> performant as Marker[] localParents
>>
>>
>> On Mon, Apr 21, 2014 at 10:07 PM, Matt Sicker <
>>
>>
>
>
> --
> Matt Sicker <bo...@gmail.com>
>
--
Bruce Brouwer
Re: Some things I noticed with the Marker implementation
Posted by Matt Sicker <bo...@gmail.com>.
On Tuesday, 22 April 2014, Bruce Brouwer <br...@gmail.com> wrote:
> I don't think performance is a real concern on this method. Style is what
> I am concerned about. My vote would be to change the add method to accept
> varargs.
>
We could include both. Or have one, two, and variable versions.
> On Apr 22, 2014 11:45 AM, "Ralph Goers" <ra...@dslextreme.com>
> wrote:
>
> I thought about the add method with multiple arguments but thought that it
> sort of goes against the builder pattern. However, I am sure it would
> perform better.
>
> Ralph
>
> On Apr 22, 2014, at 5:18 AM, Bruce Brouwer <br...@gmail.com>
> wrote:
>
> Don't worry about the part of my comment that you were confused about. I
> see now that varargs are not slower than array parameters, so long as you
> pass in an array.
>
> 1) I did find some other things to talk about. This comment:
> // It is not strictly necessary to copy the variable here but
> it should perform better than
> // Accessing a volatile variable multiple times.
> I believe should be removed. This method is not synchronized so iterating
> over an array of 5 elements while another thread removes an element would
> cause the array to shrink to 4 and cause a possible ArrayIndexOutOfBounds
> exception. I believe it is absolutely necessary to make a copy of the array.
>
> 2) Something I missed earlier regarding add that your FIXME reminded me
> of. I would like to see the add method take varargs so that I can add more
> than one parent at a time. I know the fluent nature means I could just call
> marker.add(name1).add(name2), but why not marker.add(name1, name2)?
>
> 3) public boolean isInstanceOf(final String markerName) checks for a null
> MarkerName and throws IllegalArgumentException, but the current
> implementation allows a null marker name. I believe that check should be
> removed and code it so nulls can be handled in the method. Either that or
> make the other methods throw exceptions if you pass in a null marker name.
> It is possible that adding null checks will slow this method down so I
> won't be surprised if we change other methods to disallow null marker names.
>
> 4) Please make the FIXME in isInstanceOf happen that returns false instead
> of throwing IllegalArgumentException when the marker does not exist.
>
>
>
> On Tue, Apr 22, 2014 at 12:18 AM, Matt Sicker <bo...@gmail.com> wrote:
>
> Actually, now I'm confused about that. Bruce, could you take a look at the
> current trunk version of MarkerManager?
>
>
> On 21 April 2014 21:48, Ralph Goers <rg...@apache.org> wrote:
>
> I'm not sure what you are referring to - the parameters? Using a copy of
> a volatile reference is usually faster than using the volatile reference
> itself. I think that is what Bruce is referring to.
>
> Ralph
>
> On Apr 21, 2014, at 7:20 PM, Matt Sicker <bo...@gmail.com> wrote:
>
> Using Marker... or Marker[] compile to the same bytecode. It should only
> affect compile-time stuff unless I'm mistaken.
>
>
> On 21 April 2014 20:17, Bruce Brouwer <br...@gmail.com> wrote:
>
> Yes, I did. The arrays are faster in 90% of cases. The only time I got the
> HashSet to be faster was when I was caching the entire marker hierarchy and
> the hierarchy was more than 3 levels deep. That is definitely not the most
> common case.
>
> Also, I think the Marker... localParents parameter might be more
> performant as Marker[] localParents
>
>
> On Mon, Apr 21, 2014 at 10:07 PM, Matt Sicker <
>
>
--
Matt Sicker <bo...@gmail.com>
Re: Some things I noticed with the Marker implementation
Posted by Bruce Brouwer <br...@gmail.com>.
I don't think performance is a real concern on this method. Style is what I
am concerned about. My vote would be to change the add method to accept
varargs.
On Apr 22, 2014 11:45 AM, "Ralph Goers" <ra...@dslextreme.com> wrote:
> I thought about the add method with multiple arguments but thought that it
> sort of goes against the builder pattern. However, I am sure it would
> perform better.
>
> Ralph
>
> On Apr 22, 2014, at 5:18 AM, Bruce Brouwer <br...@gmail.com>
> wrote:
>
> Don't worry about the part of my comment that you were confused about. I
> see now that varargs are not slower than array parameters, so long as you
> pass in an array.
>
> 1) I did find some other things to talk about. This comment:
> // It is not strictly necessary to copy the variable here but
> it should perform better than
> // Accessing a volatile variable multiple times.
> I believe should be removed. This method is not synchronized so iterating
> over an array of 5 elements while another thread removes an element would
> cause the array to shrink to 4 and cause a possible ArrayIndexOutOfBounds
> exception. I believe it is absolutely necessary to make a copy of the array.
>
> 2) Something I missed earlier regarding add that your FIXME reminded me
> of. I would like to see the add method take varargs so that I can add more
> than one parent at a time. I know the fluent nature means I could just call
> marker.add(name1).add(name2), but why not marker.add(name1, name2)?
>
> 3) public boolean isInstanceOf(final String markerName) checks for a null
> MarkerName and throws IllegalArgumentException, but the current
> implementation allows a null marker name. I believe that check should be
> removed and code it so nulls can be handled in the method. Either that or
> make the other methods throw exceptions if you pass in a null marker name.
> It is possible that adding null checks will slow this method down so I
> won't be surprised if we change other methods to disallow null marker names.
>
> 4) Please make the FIXME in isInstanceOf happen that returns false instead
> of throwing IllegalArgumentException when the marker does not exist.
>
>
>
> On Tue, Apr 22, 2014 at 12:18 AM, Matt Sicker <bo...@gmail.com> wrote:
>
>> Actually, now I'm confused about that. Bruce, could you take a look at
>> the current trunk version of MarkerManager?
>>
>>
>> On 21 April 2014 21:48, Ralph Goers <rg...@apache.org> wrote:
>>
>>> I'm not sure what you are referring to - the parameters? Using a copy
>>> of a volatile reference is usually faster than using the volatile reference
>>> itself. I think that is what Bruce is referring to.
>>>
>>> Ralph
>>>
>>> On Apr 21, 2014, at 7:20 PM, Matt Sicker <bo...@gmail.com> wrote:
>>>
>>> Using Marker... or Marker[] compile to the same bytecode. It should only
>>> affect compile-time stuff unless I'm mistaken.
>>>
>>>
>>> On 21 April 2014 20:17, Bruce Brouwer <br...@gmail.com> wrote:
>>>
>>>> Yes, I did. The arrays are faster in 90% of cases. The only time I got
>>>> the HashSet to be faster was when I was caching the entire marker hierarchy
>>>> and the hierarchy was more than 3 levels deep. That is definitely not the
>>>> most common case.
>>>>
>>>> Also, I think the Marker... localParents parameter might be more
>>>> performant as Marker[] localParents
>>>>
>>>>
>>>> On Mon, Apr 21, 2014 at 10:07 PM, Matt Sicker <bo...@gmail.com> wrote:
>>>>
>>>>> Also, did you compare the performance of using an array versus a
>>>>> HashSet?
>>>>>
>>>>>
>>>>> On 21 April 2014 19:44, Matt Sicker <bo...@gmail.com> wrote:
>>>>>
>>>>>> Here's what I'm changing that contains method to:
>>>>>>
>>>>>> 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;
>>>>>> }
>>>>>> }
>>>>>> return false;
>>>>>> }
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 21 April 2014 19:42, Matt Sicker <bo...@gmail.com> wrote:
>>>>>>
>>>>>>> Which brings up another issue with markers. In MarkerManager, we
>>>>>>> have a volatile array of Markers. Here's the message from IntelliJ:
>>>>>>>
>>>>>>> Reports array fields which are declared as *volatile*. Such fields
>>>>>>> may be confusing, as accessing the array itself follows the rules for
>>>>>>> *volatile* fields, but accessing the array's contents does not. If
>>>>>>> such volatile access is needed to array contents, the JDK5.0
>>>>>>> *java.util.concurrent.atomic* classes should be used instead.
>>>>>>>
>>>>>>> Is this relevant here?
>>>>>>>
>>>>>>>
>>>>>>> On 21 April 2014 19:37, Matt Sicker <bo...@gmail.com> wrote:
>>>>>>>
>>>>>>>> 1) that would be my bad. I usually replace those with foreach loops
>>>>>>>> where possible. It's usually good to comment in those cases. I'll revert
>>>>>>>> that and comment.
>>>>>>>>
>>>>>>>> 2) that makes more sense than the exception
>>>>>>>>
>>>>>>>>
>>>>>>>> On 21 April 2014 18:46, Bruce Brouwer <br...@gmail.com>wrote:
>>>>>>>>
>>>>>>>>> I saw that some small changes were being made to the Markers. I
>>>>>>>>> had a few thoughts regarding them:
>>>>>>>>>
>>>>>>>>> 1) Use of array iterator instead of indexed for loop.
>>>>>>>>> for (Marker marker : localParents)
>>>>>>>>> instead of
>>>>>>>>> for (int i = 0; i < localParents.length; i++)
>>>>>>>>>
>>>>>>>>> When I was doing my performance benchmarks, I was finding the
>>>>>>>>> latter to be faster. I'm guessing this is simply because a new Iterable
>>>>>>>>> object needs to be created to iterate over the array.
>>>>>>>>>
>>>>>>>>> For most methods, such as add, remove, this was not a big deal.
>>>>>>>>> But for the isInstanceOf and checkParent methods, we want those to be as
>>>>>>>>> fast as possible.
>>>>>>>>>
>>>>>>>>> 2) isInstanceOf(String markerName)
>>>>>>>>> Instead of throwing an IllegalArgumentException when a marker of
>>>>>>>>> name markerName doesn't exist, why don't we simply return false? I don't
>>>>>>>>> want an IllegalArgumentException to happen because I'm testing a
>>>>>>>>> markerName.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>>
>>>>>>>>> Bruce Brouwer
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Matt Sicker <bo...@gmail.com>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Matt Sicker <bo...@gmail.com>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Matt Sicker <bo...@gmail.com>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Matt Sicker <bo...@gmail.com>
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>>
>>>> Bruce Brouwer
>>>>
>>>
>>>
>>>
>>> --
>>> Matt Sicker <bo...@gmail.com>
>>>
>>>
>>
>>
>> --
>> Matt Sicker <bo...@gmail.com>
>>
>
>
>
> --
>
> Bruce Brouwer
>
>
>
Re: Some things I noticed with the Marker implementation
Posted by Ralph Goers <ra...@dslextreme.com>.
I thought about the add method with multiple arguments but thought that it sort of goes against the builder pattern. However, I am sure it would perform better.
Ralph
On Apr 22, 2014, at 5:18 AM, Bruce Brouwer <br...@gmail.com> wrote:
> Don't worry about the part of my comment that you were confused about. I see now that varargs are not slower than array parameters, so long as you pass in an array.
>
> 1) I did find some other things to talk about. This comment:
> // It is not strictly necessary to copy the variable here but it should perform better than
> // Accessing a volatile variable multiple times.
> I believe should be removed. This method is not synchronized so iterating over an array of 5 elements while another thread removes an element would cause the array to shrink to 4 and cause a possible ArrayIndexOutOfBounds exception. I believe it is absolutely necessary to make a copy of the array.
>
> 2) Something I missed earlier regarding add that your FIXME reminded me of. I would like to see the add method take varargs so that I can add more than one parent at a time. I know the fluent nature means I could just call marker.add(name1).add(name2), but why not marker.add(name1, name2)?
>
> 3) public boolean isInstanceOf(final String markerName) checks for a null MarkerName and throws IllegalArgumentException, but the current implementation allows a null marker name. I believe that check should be removed and code it so nulls can be handled in the method. Either that or make the other methods throw exceptions if you pass in a null marker name. It is possible that adding null checks will slow this method down so I won't be surprised if we change other methods to disallow null marker names.
>
> 4) Please make the FIXME in isInstanceOf happen that returns false instead of throwing IllegalArgumentException when the marker does not exist.
>
>
>
> On Tue, Apr 22, 2014 at 12:18 AM, Matt Sicker <bo...@gmail.com> wrote:
> Actually, now I'm confused about that. Bruce, could you take a look at the current trunk version of MarkerManager?
>
>
> On 21 April 2014 21:48, Ralph Goers <rg...@apache.org> wrote:
> I'm not sure what you are referring to - the parameters? Using a copy of a volatile reference is usually faster than using the volatile reference itself. I think that is what Bruce is referring to.
>
> Ralph
>
> On Apr 21, 2014, at 7:20 PM, Matt Sicker <bo...@gmail.com> wrote:
>
>> Using Marker... or Marker[] compile to the same bytecode. It should only affect compile-time stuff unless I'm mistaken.
>>
>>
>> On 21 April 2014 20:17, Bruce Brouwer <br...@gmail.com> wrote:
>> Yes, I did. The arrays are faster in 90% of cases. The only time I got the HashSet to be faster was when I was caching the entire marker hierarchy and the hierarchy was more than 3 levels deep. That is definitely not the most common case.
>>
>> Also, I think the Marker... localParents parameter might be more performant as Marker[] localParents
>>
>>
>> On Mon, Apr 21, 2014 at 10:07 PM, Matt Sicker <bo...@gmail.com> wrote:
>> Also, did you compare the performance of using an array versus a HashSet?
>>
>>
>> On 21 April 2014 19:44, Matt Sicker <bo...@gmail.com> wrote:
>> Here's what I'm changing that contains method to:
>>
>> 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;
>> }
>> }
>> return false;
>> }
>>
>>
>>
>> On 21 April 2014 19:42, Matt Sicker <bo...@gmail.com> wrote:
>> Which brings up another issue with markers. In MarkerManager, we have a volatile array of Markers. Here's the message from IntelliJ:
>>
>> Reports array fields which are declared as volatile. Such fields may be confusing, as accessing the array itself follows the rules for volatile fields, but accessing the array's contents does not. If such volatile access is needed to array contents, the JDK5.0 java.util.concurrent.atomic classes should be used instead.
>>
>> Is this relevant here?
>>
>>
>> On 21 April 2014 19:37, Matt Sicker <bo...@gmail.com> wrote:
>> 1) that would be my bad. I usually replace those with foreach loops where possible. It's usually good to comment in those cases. I'll revert that and comment.
>>
>> 2) that makes more sense than the exception
>>
>>
>> On 21 April 2014 18:46, Bruce Brouwer <br...@gmail.com> wrote:
>> I saw that some small changes were being made to the Markers. I had a few thoughts regarding them:
>>
>> 1) Use of array iterator instead of indexed for loop.
>> for (Marker marker : localParents)
>> instead of
>> for (int i = 0; i < localParents.length; i++)
>>
>> When I was doing my performance benchmarks, I was finding the latter to be faster. I'm guessing this is simply because a new Iterable object needs to be created to iterate over the array.
>>
>> For most methods, such as add, remove, this was not a big deal. But for the isInstanceOf and checkParent methods, we want those to be as fast as possible.
>>
>> 2) isInstanceOf(String markerName)
>> Instead of throwing an IllegalArgumentException when a marker of name markerName doesn't exist, why don't we simply return false? I don't want an IllegalArgumentException to happen because I'm testing a markerName.
>>
>>
>>
>> --
>>
>> Bruce Brouwer
>>
>>
>>
>> --
>> Matt Sicker <bo...@gmail.com>
>>
>>
>>
>> --
>> Matt Sicker <bo...@gmail.com>
>>
>>
>>
>> --
>> Matt Sicker <bo...@gmail.com>
>>
>>
>>
>> --
>> Matt Sicker <bo...@gmail.com>
>>
>>
>>
>> --
>>
>> Bruce Brouwer
>>
>>
>>
>> --
>> Matt Sicker <bo...@gmail.com>
>
>
>
> --
> Matt Sicker <bo...@gmail.com>
>
>
>
> --
>
> Bruce Brouwer
Re: Some things I noticed with the Marker implementation
Posted by Bruce Brouwer <br...@gmail.com>.
Don't worry about the part of my comment that you were confused about. I
see now that varargs are not slower than array parameters, so long as you
pass in an array.
1) I did find some other things to talk about. This comment:
// It is not strictly necessary to copy the variable here but
it should perform better than
// Accessing a volatile variable multiple times.
I believe should be removed. This method is not synchronized so iterating
over an array of 5 elements while another thread removes an element would
cause the array to shrink to 4 and cause a possible ArrayIndexOutOfBounds
exception. I believe it is absolutely necessary to make a copy of the array.
2) Something I missed earlier regarding add that your FIXME reminded me of.
I would like to see the add method take varargs so that I can add more than
one parent at a time. I know the fluent nature means I could just call
marker.add(name1).add(name2), but why not marker.add(name1, name2)?
3) public boolean isInstanceOf(final String markerName) checks for a null
MarkerName and throws IllegalArgumentException, but the current
implementation allows a null marker name. I believe that check should be
removed and code it so nulls can be handled in the method. Either that or
make the other methods throw exceptions if you pass in a null marker name.
It is possible that adding null checks will slow this method down so I
won't be surprised if we change other methods to disallow null marker names.
4) Please make the FIXME in isInstanceOf happen that returns false instead
of throwing IllegalArgumentException when the marker does not exist.
On Tue, Apr 22, 2014 at 12:18 AM, Matt Sicker <bo...@gmail.com> wrote:
> Actually, now I'm confused about that. Bruce, could you take a look at the
> current trunk version of MarkerManager?
>
>
> On 21 April 2014 21:48, Ralph Goers <rg...@apache.org> wrote:
>
>> I'm not sure what you are referring to - the parameters? Using a copy of
>> a volatile reference is usually faster than using the volatile reference
>> itself. I think that is what Bruce is referring to.
>>
>> Ralph
>>
>> On Apr 21, 2014, at 7:20 PM, Matt Sicker <bo...@gmail.com> wrote:
>>
>> Using Marker... or Marker[] compile to the same bytecode. It should only
>> affect compile-time stuff unless I'm mistaken.
>>
>>
>> On 21 April 2014 20:17, Bruce Brouwer <br...@gmail.com> wrote:
>>
>>> Yes, I did. The arrays are faster in 90% of cases. The only time I got
>>> the HashSet to be faster was when I was caching the entire marker hierarchy
>>> and the hierarchy was more than 3 levels deep. That is definitely not the
>>> most common case.
>>>
>>> Also, I think the Marker... localParents parameter might be more
>>> performant as Marker[] localParents
>>>
>>>
>>> On Mon, Apr 21, 2014 at 10:07 PM, Matt Sicker <bo...@gmail.com> wrote:
>>>
>>>> Also, did you compare the performance of using an array versus a
>>>> HashSet?
>>>>
>>>>
>>>> On 21 April 2014 19:44, Matt Sicker <bo...@gmail.com> wrote:
>>>>
>>>>> Here's what I'm changing that contains method to:
>>>>>
>>>>> 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;
>>>>> }
>>>>> }
>>>>> return false;
>>>>> }
>>>>>
>>>>>
>>>>>
>>>>> On 21 April 2014 19:42, Matt Sicker <bo...@gmail.com> wrote:
>>>>>
>>>>>> Which brings up another issue with markers. In MarkerManager, we have
>>>>>> a volatile array of Markers. Here's the message from IntelliJ:
>>>>>>
>>>>>> Reports array fields which are declared as *volatile*. Such fields
>>>>>> may be confusing, as accessing the array itself follows the rules for
>>>>>> *volatile* fields, but accessing the array's contents does not. If
>>>>>> such volatile access is needed to array contents, the JDK5.0
>>>>>> *java.util.concurrent.atomic* classes should be used instead.
>>>>>>
>>>>>> Is this relevant here?
>>>>>>
>>>>>>
>>>>>> On 21 April 2014 19:37, Matt Sicker <bo...@gmail.com> wrote:
>>>>>>
>>>>>>> 1) that would be my bad. I usually replace those with foreach loops
>>>>>>> where possible. It's usually good to comment in those cases. I'll revert
>>>>>>> that and comment.
>>>>>>>
>>>>>>> 2) that makes more sense than the exception
>>>>>>>
>>>>>>>
>>>>>>> On 21 April 2014 18:46, Bruce Brouwer <br...@gmail.com>wrote:
>>>>>>>
>>>>>>>> I saw that some small changes were being made to the Markers. I had
>>>>>>>> a few thoughts regarding them:
>>>>>>>>
>>>>>>>> 1) Use of array iterator instead of indexed for loop.
>>>>>>>> for (Marker marker : localParents)
>>>>>>>> instead of
>>>>>>>> for (int i = 0; i < localParents.length; i++)
>>>>>>>>
>>>>>>>> When I was doing my performance benchmarks, I was finding the
>>>>>>>> latter to be faster. I'm guessing this is simply because a new Iterable
>>>>>>>> object needs to be created to iterate over the array.
>>>>>>>>
>>>>>>>> For most methods, such as add, remove, this was not a big deal. But
>>>>>>>> for the isInstanceOf and checkParent methods, we want those to be as fast
>>>>>>>> as possible.
>>>>>>>>
>>>>>>>> 2) isInstanceOf(String markerName)
>>>>>>>> Instead of throwing an IllegalArgumentException when a marker of
>>>>>>>> name markerName doesn't exist, why don't we simply return false? I don't
>>>>>>>> want an IllegalArgumentException to happen because I'm testing a
>>>>>>>> markerName.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>>
>>>>>>>> Bruce Brouwer
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Matt Sicker <bo...@gmail.com>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Matt Sicker <bo...@gmail.com>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Matt Sicker <bo...@gmail.com>
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Matt Sicker <bo...@gmail.com>
>>>>
>>>
>>>
>>>
>>> --
>>>
>>> Bruce Brouwer
>>>
>>
>>
>>
>> --
>> Matt Sicker <bo...@gmail.com>
>>
>>
>
>
> --
> Matt Sicker <bo...@gmail.com>
>
--
Bruce Brouwer
Re: Some things I noticed with the Marker implementation
Posted by Matt Sicker <bo...@gmail.com>.
Actually, now I'm confused about that. Bruce, could you take a look at the
current trunk version of MarkerManager?
On 21 April 2014 21:48, Ralph Goers <rg...@apache.org> wrote:
> I'm not sure what you are referring to - the parameters? Using a copy of
> a volatile reference is usually faster than using the volatile reference
> itself. I think that is what Bruce is referring to.
>
> Ralph
>
> On Apr 21, 2014, at 7:20 PM, Matt Sicker <bo...@gmail.com> wrote:
>
> Using Marker... or Marker[] compile to the same bytecode. It should only
> affect compile-time stuff unless I'm mistaken.
>
>
> On 21 April 2014 20:17, Bruce Brouwer <br...@gmail.com> wrote:
>
>> Yes, I did. The arrays are faster in 90% of cases. The only time I got
>> the HashSet to be faster was when I was caching the entire marker hierarchy
>> and the hierarchy was more than 3 levels deep. That is definitely not the
>> most common case.
>>
>> Also, I think the Marker... localParents parameter might be more
>> performant as Marker[] localParents
>>
>>
>> On Mon, Apr 21, 2014 at 10:07 PM, Matt Sicker <bo...@gmail.com> wrote:
>>
>>> Also, did you compare the performance of using an array versus a HashSet?
>>>
>>>
>>> On 21 April 2014 19:44, Matt Sicker <bo...@gmail.com> wrote:
>>>
>>>> Here's what I'm changing that contains method to:
>>>>
>>>> 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;
>>>> }
>>>> }
>>>> return false;
>>>> }
>>>>
>>>>
>>>>
>>>> On 21 April 2014 19:42, Matt Sicker <bo...@gmail.com> wrote:
>>>>
>>>>> Which brings up another issue with markers. In MarkerManager, we have
>>>>> a volatile array of Markers. Here's the message from IntelliJ:
>>>>>
>>>>> Reports array fields which are declared as *volatile*. Such fields
>>>>> may be confusing, as accessing the array itself follows the rules for
>>>>> *volatile* fields, but accessing the array's contents does not. If
>>>>> such volatile access is needed to array contents, the JDK5.0
>>>>> *java.util.concurrent.atomic* classes should be used instead.
>>>>>
>>>>> Is this relevant here?
>>>>>
>>>>>
>>>>> On 21 April 2014 19:37, Matt Sicker <bo...@gmail.com> wrote:
>>>>>
>>>>>> 1) that would be my bad. I usually replace those with foreach loops
>>>>>> where possible. It's usually good to comment in those cases. I'll revert
>>>>>> that and comment.
>>>>>>
>>>>>> 2) that makes more sense than the exception
>>>>>>
>>>>>>
>>>>>> On 21 April 2014 18:46, Bruce Brouwer <br...@gmail.com>wrote:
>>>>>>
>>>>>>> I saw that some small changes were being made to the Markers. I had
>>>>>>> a few thoughts regarding them:
>>>>>>>
>>>>>>> 1) Use of array iterator instead of indexed for loop.
>>>>>>> for (Marker marker : localParents)
>>>>>>> instead of
>>>>>>> for (int i = 0; i < localParents.length; i++)
>>>>>>>
>>>>>>> When I was doing my performance benchmarks, I was finding the latter
>>>>>>> to be faster. I'm guessing this is simply because a new Iterable object
>>>>>>> needs to be created to iterate over the array.
>>>>>>>
>>>>>>> For most methods, such as add, remove, this was not a big deal. But
>>>>>>> for the isInstanceOf and checkParent methods, we want those to be as fast
>>>>>>> as possible.
>>>>>>>
>>>>>>> 2) isInstanceOf(String markerName)
>>>>>>> Instead of throwing an IllegalArgumentException when a marker of
>>>>>>> name markerName doesn't exist, why don't we simply return false? I don't
>>>>>>> want an IllegalArgumentException to happen because I'm testing a
>>>>>>> markerName.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>>
>>>>>>> Bruce Brouwer
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Matt Sicker <bo...@gmail.com>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Matt Sicker <bo...@gmail.com>
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Matt Sicker <bo...@gmail.com>
>>>>
>>>
>>>
>>>
>>> --
>>> Matt Sicker <bo...@gmail.com>
>>>
>>
>>
>>
>> --
>>
>> Bruce Brouwer
>>
>
>
>
> --
> Matt Sicker <bo...@gmail.com>
>
>
--
Matt Sicker <bo...@gmail.com>
Re: Some things I noticed with the Marker implementation
Posted by Ralph Goers <rg...@apache.org>.
I'm not sure what you are referring to - the parameters? Using a copy of a volatile reference is usually faster than using the volatile reference itself. I think that is what Bruce is referring to.
Ralph
> On Apr 21, 2014, at 7:20 PM, Matt Sicker <bo...@gmail.com> wrote:
>
> Using Marker... or Marker[] compile to the same bytecode. It should only affect compile-time stuff unless I'm mistaken.
>
>
>> On 21 April 2014 20:17, Bruce Brouwer <br...@gmail.com> wrote:
>> Yes, I did. The arrays are faster in 90% of cases. The only time I got the HashSet to be faster was when I was caching the entire marker hierarchy and the hierarchy was more than 3 levels deep. That is definitely not the most common case.
>>
>> Also, I think the Marker... localParents parameter might be more performant as Marker[] localParents
>>
>>
>>> On Mon, Apr 21, 2014 at 10:07 PM, Matt Sicker <bo...@gmail.com> wrote:
>>> Also, did you compare the performance of using an array versus a HashSet?
>>>
>>>
>>>> On 21 April 2014 19:44, Matt Sicker <bo...@gmail.com> wrote:
>>>> Here's what I'm changing that contains method to:
>>>>
>>>> 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;
>>>> }
>>>> }
>>>> return false;
>>>> }
>>>>
>>>>
>>>>
>>>>> On 21 April 2014 19:42, Matt Sicker <bo...@gmail.com> wrote:
>>>>> Which brings up another issue with markers. In MarkerManager, we have a volatile array of Markers. Here's the message from IntelliJ:
>>>>>
>>>>> Reports array fields which are declared as volatile. Such fields may be confusing, as accessing the array itself follows the rules for volatile fields, but accessing the array's contents does not. If such volatile access is needed to array contents, the JDK5.0 java.util.concurrent.atomic classes should be used instead.
>>>>>
>>>>> Is this relevant here?
>>>>>
>>>>>
>>>>>> On 21 April 2014 19:37, Matt Sicker <bo...@gmail.com> wrote:
>>>>>> 1) that would be my bad. I usually replace those with foreach loops where possible. It's usually good to comment in those cases. I'll revert that and comment.
>>>>>>
>>>>>> 2) that makes more sense than the exception
>>>>>>
>>>>>>
>>>>>>> On 21 April 2014 18:46, Bruce Brouwer <br...@gmail.com> wrote:
>>>>>>> I saw that some small changes were being made to the Markers. I had a few thoughts regarding them:
>>>>>>>
>>>>>>> 1) Use of array iterator instead of indexed for loop.
>>>>>>> for (Marker marker : localParents)
>>>>>>> instead of
>>>>>>> for (int i = 0; i < localParents.length; i++)
>>>>>>>
>>>>>>> When I was doing my performance benchmarks, I was finding the latter to be faster. I'm guessing this is simply because a new Iterable object needs to be created to iterate over the array.
>>>>>>>
>>>>>>> For most methods, such as add, remove, this was not a big deal. But for the isInstanceOf and checkParent methods, we want those to be as fast as possible.
>>>>>>>
>>>>>>> 2) isInstanceOf(String markerName)
>>>>>>> Instead of throwing an IllegalArgumentException when a marker of name markerName doesn't exist, why don't we simply return false? I don't want an IllegalArgumentException to happen because I'm testing a markerName.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>>
>>>>>>> Bruce Brouwer
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Matt Sicker <bo...@gmail.com>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Matt Sicker <bo...@gmail.com>
>>>>
>>>>
>>>>
>>>> --
>>>> Matt Sicker <bo...@gmail.com>
>>>
>>>
>>>
>>> --
>>> Matt Sicker <bo...@gmail.com>
>>
>>
>>
>> --
>>
>> Bruce Brouwer
>
>
>
> --
> Matt Sicker <bo...@gmail.com>
Re: Some things I noticed with the Marker implementation
Posted by Matt Sicker <bo...@gmail.com>.
Using Marker... or Marker[] compile to the same bytecode. It should only
affect compile-time stuff unless I'm mistaken.
On 21 April 2014 20:17, Bruce Brouwer <br...@gmail.com> wrote:
> Yes, I did. The arrays are faster in 90% of cases. The only time I got the
> HashSet to be faster was when I was caching the entire marker hierarchy and
> the hierarchy was more than 3 levels deep. That is definitely not the most
> common case.
>
> Also, I think the Marker... localParents parameter might be more
> performant as Marker[] localParents
>
>
> On Mon, Apr 21, 2014 at 10:07 PM, Matt Sicker <bo...@gmail.com> wrote:
>
>> Also, did you compare the performance of using an array versus a HashSet?
>>
>>
>> On 21 April 2014 19:44, Matt Sicker <bo...@gmail.com> wrote:
>>
>>> Here's what I'm changing that contains method to:
>>>
>>> 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;
>>> }
>>> }
>>> return false;
>>> }
>>>
>>>
>>>
>>> On 21 April 2014 19:42, Matt Sicker <bo...@gmail.com> wrote:
>>>
>>>> Which brings up another issue with markers. In MarkerManager, we have a
>>>> volatile array of Markers. Here's the message from IntelliJ:
>>>>
>>>> Reports array fields which are declared as *volatile*. Such fields may
>>>> be confusing, as accessing the array itself follows the rules for
>>>> *volatile* fields, but accessing the array's contents does not. If
>>>> such volatile access is needed to array contents, the JDK5.0
>>>> *java.util.concurrent.atomic* classes should be used instead.
>>>>
>>>> Is this relevant here?
>>>>
>>>>
>>>> On 21 April 2014 19:37, Matt Sicker <bo...@gmail.com> wrote:
>>>>
>>>>> 1) that would be my bad. I usually replace those with foreach loops
>>>>> where possible. It's usually good to comment in those cases. I'll revert
>>>>> that and comment.
>>>>>
>>>>> 2) that makes more sense than the exception
>>>>>
>>>>>
>>>>> On 21 April 2014 18:46, Bruce Brouwer <br...@gmail.com> wrote:
>>>>>
>>>>>> I saw that some small changes were being made to the Markers. I had a
>>>>>> few thoughts regarding them:
>>>>>>
>>>>>> 1) Use of array iterator instead of indexed for loop.
>>>>>> for (Marker marker : localParents)
>>>>>> instead of
>>>>>> for (int i = 0; i < localParents.length; i++)
>>>>>>
>>>>>> When I was doing my performance benchmarks, I was finding the latter
>>>>>> to be faster. I'm guessing this is simply because a new Iterable object
>>>>>> needs to be created to iterate over the array.
>>>>>>
>>>>>> For most methods, such as add, remove, this was not a big deal. But
>>>>>> for the isInstanceOf and checkParent methods, we want those to be as fast
>>>>>> as possible.
>>>>>>
>>>>>> 2) isInstanceOf(String markerName)
>>>>>> Instead of throwing an IllegalArgumentException when a marker of name
>>>>>> markerName doesn't exist, why don't we simply return false? I don't want an
>>>>>> IllegalArgumentException to happen because I'm testing a markerName.
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>>
>>>>>> Bruce Brouwer
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Matt Sicker <bo...@gmail.com>
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Matt Sicker <bo...@gmail.com>
>>>>
>>>
>>>
>>>
>>> --
>>> Matt Sicker <bo...@gmail.com>
>>>
>>
>>
>>
>> --
>> Matt Sicker <bo...@gmail.com>
>>
>
>
>
> --
>
> Bruce Brouwer
>
--
Matt Sicker <bo...@gmail.com>
Re: Some things I noticed with the Marker implementation
Posted by Bruce Brouwer <br...@gmail.com>.
Yes, I did. The arrays are faster in 90% of cases. The only time I got the
HashSet to be faster was when I was caching the entire marker hierarchy and
the hierarchy was more than 3 levels deep. That is definitely not the most
common case.
Also, I think the Marker... localParents parameter might be more performant
as Marker[] localParents
On Mon, Apr 21, 2014 at 10:07 PM, Matt Sicker <bo...@gmail.com> wrote:
> Also, did you compare the performance of using an array versus a HashSet?
>
>
> On 21 April 2014 19:44, Matt Sicker <bo...@gmail.com> wrote:
>
>> Here's what I'm changing that contains method to:
>>
>> 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;
>> }
>> }
>> return false;
>> }
>>
>>
>>
>> On 21 April 2014 19:42, Matt Sicker <bo...@gmail.com> wrote:
>>
>>> Which brings up another issue with markers. In MarkerManager, we have a
>>> volatile array of Markers. Here's the message from IntelliJ:
>>>
>>> Reports array fields which are declared as *volatile*. Such fields may
>>> be confusing, as accessing the array itself follows the rules for
>>> *volatile* fields, but accessing the array's contents does not. If such
>>> volatile access is needed to array contents, the JDK5.0
>>> *java.util.concurrent.atomic* classes should be used instead.
>>>
>>> Is this relevant here?
>>>
>>>
>>> On 21 April 2014 19:37, Matt Sicker <bo...@gmail.com> wrote:
>>>
>>>> 1) that would be my bad. I usually replace those with foreach loops
>>>> where possible. It's usually good to comment in those cases. I'll revert
>>>> that and comment.
>>>>
>>>> 2) that makes more sense than the exception
>>>>
>>>>
>>>> On 21 April 2014 18:46, Bruce Brouwer <br...@gmail.com> wrote:
>>>>
>>>>> I saw that some small changes were being made to the Markers. I had a
>>>>> few thoughts regarding them:
>>>>>
>>>>> 1) Use of array iterator instead of indexed for loop.
>>>>> for (Marker marker : localParents)
>>>>> instead of
>>>>> for (int i = 0; i < localParents.length; i++)
>>>>>
>>>>> When I was doing my performance benchmarks, I was finding the latter
>>>>> to be faster. I'm guessing this is simply because a new Iterable object
>>>>> needs to be created to iterate over the array.
>>>>>
>>>>> For most methods, such as add, remove, this was not a big deal. But
>>>>> for the isInstanceOf and checkParent methods, we want those to be as fast
>>>>> as possible.
>>>>>
>>>>> 2) isInstanceOf(String markerName)
>>>>> Instead of throwing an IllegalArgumentException when a marker of name
>>>>> markerName doesn't exist, why don't we simply return false? I don't want an
>>>>> IllegalArgumentException to happen because I'm testing a markerName.
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>>
>>>>> Bruce Brouwer
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Matt Sicker <bo...@gmail.com>
>>>>
>>>
>>>
>>>
>>> --
>>> Matt Sicker <bo...@gmail.com>
>>>
>>
>>
>>
>> --
>> Matt Sicker <bo...@gmail.com>
>>
>
>
>
> --
> Matt Sicker <bo...@gmail.com>
>
--
Bruce Brouwer
Re: Some things I noticed with the Marker implementation
Posted by Matt Sicker <bo...@gmail.com>.
Also, did you compare the performance of using an array versus a HashSet?
On 21 April 2014 19:44, Matt Sicker <bo...@gmail.com> wrote:
> Here's what I'm changing that contains method to:
>
> 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;
> }
> }
> return false;
> }
>
>
>
> On 21 April 2014 19:42, Matt Sicker <bo...@gmail.com> wrote:
>
>> Which brings up another issue with markers. In MarkerManager, we have a
>> volatile array of Markers. Here's the message from IntelliJ:
>>
>> Reports array fields which are declared as *volatile*. Such fields may
>> be confusing, as accessing the array itself follows the rules for
>> *volatile* fields, but accessing the array's contents does not. If such
>> volatile access is needed to array contents, the JDK5.0
>> *java.util.concurrent.atomic* classes should be used instead.
>>
>> Is this relevant here?
>>
>>
>> On 21 April 2014 19:37, Matt Sicker <bo...@gmail.com> wrote:
>>
>>> 1) that would be my bad. I usually replace those with foreach loops
>>> where possible. It's usually good to comment in those cases. I'll revert
>>> that and comment.
>>>
>>> 2) that makes more sense than the exception
>>>
>>>
>>> On 21 April 2014 18:46, Bruce Brouwer <br...@gmail.com> wrote:
>>>
>>>> I saw that some small changes were being made to the Markers. I had a
>>>> few thoughts regarding them:
>>>>
>>>> 1) Use of array iterator instead of indexed for loop.
>>>> for (Marker marker : localParents)
>>>> instead of
>>>> for (int i = 0; i < localParents.length; i++)
>>>>
>>>> When I was doing my performance benchmarks, I was finding the latter to
>>>> be faster. I'm guessing this is simply because a new Iterable object needs
>>>> to be created to iterate over the array.
>>>>
>>>> For most methods, such as add, remove, this was not a big deal. But for
>>>> the isInstanceOf and checkParent methods, we want those to be as fast as
>>>> possible.
>>>>
>>>> 2) isInstanceOf(String markerName)
>>>> Instead of throwing an IllegalArgumentException when a marker of name
>>>> markerName doesn't exist, why don't we simply return false? I don't want an
>>>> IllegalArgumentException to happen because I'm testing a markerName.
>>>>
>>>>
>>>>
>>>> --
>>>>
>>>> Bruce Brouwer
>>>>
>>>
>>>
>>>
>>> --
>>> Matt Sicker <bo...@gmail.com>
>>>
>>
>>
>>
>> --
>> Matt Sicker <bo...@gmail.com>
>>
>
>
>
> --
> Matt Sicker <bo...@gmail.com>
>
--
Matt Sicker <bo...@gmail.com>
Re: Some things I noticed with the Marker implementation
Posted by Matt Sicker <bo...@gmail.com>.
Here's what I'm changing that contains method to:
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;
}
}
return false;
}
On 21 April 2014 19:42, Matt Sicker <bo...@gmail.com> wrote:
> Which brings up another issue with markers. In MarkerManager, we have a
> volatile array of Markers. Here's the message from IntelliJ:
>
> Reports array fields which are declared as *volatile*. Such fields may be
> confusing, as accessing the array itself follows the rules for *volatile*fields, but accessing the array's contents does not. If such volatile
> access is needed to array contents, the JDK5.0
> *java.util.concurrent.atomic* classes should be used instead.
>
> Is this relevant here?
>
>
> On 21 April 2014 19:37, Matt Sicker <bo...@gmail.com> wrote:
>
>> 1) that would be my bad. I usually replace those with foreach loops where
>> possible. It's usually good to comment in those cases. I'll revert that and
>> comment.
>>
>> 2) that makes more sense than the exception
>>
>>
>> On 21 April 2014 18:46, Bruce Brouwer <br...@gmail.com> wrote:
>>
>>> I saw that some small changes were being made to the Markers. I had a
>>> few thoughts regarding them:
>>>
>>> 1) Use of array iterator instead of indexed for loop.
>>> for (Marker marker : localParents)
>>> instead of
>>> for (int i = 0; i < localParents.length; i++)
>>>
>>> When I was doing my performance benchmarks, I was finding the latter to
>>> be faster. I'm guessing this is simply because a new Iterable object needs
>>> to be created to iterate over the array.
>>>
>>> For most methods, such as add, remove, this was not a big deal. But for
>>> the isInstanceOf and checkParent methods, we want those to be as fast as
>>> possible.
>>>
>>> 2) isInstanceOf(String markerName)
>>> Instead of throwing an IllegalArgumentException when a marker of name
>>> markerName doesn't exist, why don't we simply return false? I don't want an
>>> IllegalArgumentException to happen because I'm testing a markerName.
>>>
>>>
>>>
>>> --
>>>
>>> Bruce Brouwer
>>>
>>
>>
>>
>> --
>> Matt Sicker <bo...@gmail.com>
>>
>
>
>
> --
> Matt Sicker <bo...@gmail.com>
>
--
Matt Sicker <bo...@gmail.com>
Re: Some things I noticed with the Marker implementation
Posted by Matt Sicker <bo...@gmail.com>.
OK, that's what I thought. Thanks for confirming!
On 21 April 2014 21:46, Ralph Goers <rg...@apache.org> wrote:
> We only modify the array reference. A new array is created whenever it is
> modified so volatile is correct here.
>
> Ralph
>
> On Apr 21, 2014, at 6:42 PM, Matt Sicker <bo...@gmail.com> wrote:
>
> Which brings up another issue with markers. In MarkerManager, we have a
> volatile array of Markers. Here's the message from IntelliJ:
>
> Reports array fields which are declared as *volatile*. Such fields may be
> confusing, as accessing the array itself follows the rules for *volatile*fields, but accessing the array's contents does not. If such volatile
> access is needed to array contents, the JDK5.0
> *java.util.concurrent.atomic* classes should be used instead.
>
> Is this relevant here?
>
>
> On 21 April 2014 19:37, Matt Sicker <bo...@gmail.com> wrote:
>
>> 1) that would be my bad. I usually replace those with foreach loops where
>> possible. It's usually good to comment in those cases. I'll revert that and
>> comment.
>>
>> 2) that makes more sense than the exception
>>
>>
>> On 21 April 2014 18:46, Bruce Brouwer <br...@gmail.com> wrote:
>>
>>> I saw that some small changes were being made to the Markers. I had a
>>> few thoughts regarding them:
>>>
>>> 1) Use of array iterator instead of indexed for loop.
>>> for (Marker marker : localParents)
>>> instead of
>>> for (int i = 0; i < localParents.length; i++)
>>>
>>> When I was doing my performance benchmarks, I was finding the latter to
>>> be faster. I'm guessing this is simply because a new Iterable object needs
>>> to be created to iterate over the array.
>>>
>>> For most methods, such as add, remove, this was not a big deal. But for
>>> the isInstanceOf and checkParent methods, we want those to be as fast as
>>> possible.
>>>
>>> 2) isInstanceOf(String markerName)
>>> Instead of throwing an IllegalArgumentException when a marker of name
>>> markerName doesn't exist, why don't we simply return false? I don't want an
>>> IllegalArgumentException to happen because I'm testing a markerName.
>>>
>>>
>>>
>>> --
>>>
>>> Bruce Brouwer
>>>
>>
>>
>>
>> --
>> Matt Sicker <bo...@gmail.com>
>>
>
>
>
> --
> Matt Sicker <bo...@gmail.com>
>
>
--
Matt Sicker <bo...@gmail.com>
Re: Some things I noticed with the Marker implementation
Posted by Ralph Goers <rg...@apache.org>.
We only modify the array reference. A new array is created whenever it is modified so volatile is correct here.
Ralph
> On Apr 21, 2014, at 6:42 PM, Matt Sicker <bo...@gmail.com> wrote:
>
> Which brings up another issue with markers. In MarkerManager, we have a volatile array of Markers. Here's the message from IntelliJ:
>
> Reports array fields which are declared as volatile. Such fields may be confusing, as accessing the array itself follows the rules for volatile fields, but accessing the array's contents does not. If such volatile access is needed to array contents, the JDK5.0 java.util.concurrent.atomic classes should be used instead.
>
> Is this relevant here?
>
>
>> On 21 April 2014 19:37, Matt Sicker <bo...@gmail.com> wrote:
>> 1) that would be my bad. I usually replace those with foreach loops where possible. It's usually good to comment in those cases. I'll revert that and comment.
>>
>> 2) that makes more sense than the exception
>>
>>
>>> On 21 April 2014 18:46, Bruce Brouwer <br...@gmail.com> wrote:
>>> I saw that some small changes were being made to the Markers. I had a few thoughts regarding them:
>>>
>>> 1) Use of array iterator instead of indexed for loop.
>>> for (Marker marker : localParents)
>>> instead of
>>> for (int i = 0; i < localParents.length; i++)
>>>
>>> When I was doing my performance benchmarks, I was finding the latter to be faster. I'm guessing this is simply because a new Iterable object needs to be created to iterate over the array.
>>>
>>> For most methods, such as add, remove, this was not a big deal. But for the isInstanceOf and checkParent methods, we want those to be as fast as possible.
>>>
>>> 2) isInstanceOf(String markerName)
>>> Instead of throwing an IllegalArgumentException when a marker of name markerName doesn't exist, why don't we simply return false? I don't want an IllegalArgumentException to happen because I'm testing a markerName.
>>>
>>>
>>>
>>> --
>>>
>>> Bruce Brouwer
>>
>>
>>
>> --
>> Matt Sicker <bo...@gmail.com>
>
>
>
> --
> Matt Sicker <bo...@gmail.com>
Re: Some things I noticed with the Marker implementation
Posted by Matt Sicker <bo...@gmail.com>.
Which brings up another issue with markers. In MarkerManager, we have a
volatile array of Markers. Here's the message from IntelliJ:
Reports array fields which are declared as *volatile*. Such fields may be
confusing, as accessing the array itself follows the rules for
*volatile*fields, but accessing the array's contents does not. If such
volatile
access is needed to array contents, the JDK5.0
*java.util.concurrent.atomic*classes should be used instead.
Is this relevant here?
On 21 April 2014 19:37, Matt Sicker <bo...@gmail.com> wrote:
> 1) that would be my bad. I usually replace those with foreach loops where
> possible. It's usually good to comment in those cases. I'll revert that and
> comment.
>
> 2) that makes more sense than the exception
>
>
> On 21 April 2014 18:46, Bruce Brouwer <br...@gmail.com> wrote:
>
>> I saw that some small changes were being made to the Markers. I had a few
>> thoughts regarding them:
>>
>> 1) Use of array iterator instead of indexed for loop.
>> for (Marker marker : localParents)
>> instead of
>> for (int i = 0; i < localParents.length; i++)
>>
>> When I was doing my performance benchmarks, I was finding the latter to
>> be faster. I'm guessing this is simply because a new Iterable object needs
>> to be created to iterate over the array.
>>
>> For most methods, such as add, remove, this was not a big deal. But for
>> the isInstanceOf and checkParent methods, we want those to be as fast as
>> possible.
>>
>> 2) isInstanceOf(String markerName)
>> Instead of throwing an IllegalArgumentException when a marker of name
>> markerName doesn't exist, why don't we simply return false? I don't want an
>> IllegalArgumentException to happen because I'm testing a markerName.
>>
>>
>>
>> --
>>
>> Bruce Brouwer
>>
>
>
>
> --
> Matt Sicker <bo...@gmail.com>
>
--
Matt Sicker <bo...@gmail.com>
Re: Some things I noticed with the Marker implementation
Posted by Matt Sicker <bo...@gmail.com>.
1) that would be my bad. I usually replace those with foreach loops where
possible. It's usually good to comment in those cases. I'll revert that and
comment.
2) that makes more sense than the exception
On 21 April 2014 18:46, Bruce Brouwer <br...@gmail.com> wrote:
> I saw that some small changes were being made to the Markers. I had a few
> thoughts regarding them:
>
> 1) Use of array iterator instead of indexed for loop.
> for (Marker marker : localParents)
> instead of
> for (int i = 0; i < localParents.length; i++)
>
> When I was doing my performance benchmarks, I was finding the latter to be
> faster. I'm guessing this is simply because a new Iterable object needs to
> be created to iterate over the array.
>
> For most methods, such as add, remove, this was not a big deal. But for
> the isInstanceOf and checkParent methods, we want those to be as fast as
> possible.
>
> 2) isInstanceOf(String markerName)
> Instead of throwing an IllegalArgumentException when a marker of name
> markerName doesn't exist, why don't we simply return false? I don't want an
> IllegalArgumentException to happen because I'm testing a markerName.
>
>
>
> --
>
> Bruce Brouwer
>
--
Matt Sicker <bo...@gmail.com>
Re: Some things I noticed with the Marker implementation
Posted by Ralph Goers <rg...@apache.org>.
Yes, my benchmark showed the same thing, which is why I used the index.
Ralph
> On Apr 21, 2014, at 5:46 PM, Bruce Brouwer <br...@gmail.com> wrote:
>
> I saw that some small changes were being made to the Markers. I had a few thoughts regarding them:
>
> 1) Use of array iterator instead of indexed for loop.
> for (Marker marker : localParents)
> instead of
> for (int i = 0; i < localParents.length; i++)
>
> When I was doing my performance benchmarks, I was finding the latter to be faster. I'm guessing this is simply because a new Iterable object needs to be created to iterate over the array.
>
> For most methods, such as add, remove, this was not a big deal. But for the isInstanceOf and checkParent methods, we want those to be as fast as possible.
>
> 2) isInstanceOf(String markerName)
> Instead of throwing an IllegalArgumentException when a marker of name markerName doesn't exist, why don't we simply return false? I don't want an IllegalArgumentException to happen because I'm testing a markerName.
>
>
>
> --
>
> Bruce Brouwer
---------------------------------------------------------------------
To unsubscribe, e-mail: log4j-dev-unsubscribe@logging.apache.org
For additional commands, e-mail: log4j-dev-help@logging.apache.org