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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
>>>
>>>> Also, did you compare the performance of using an array versus a
>>>> HashSet?
>>>>
>>>>
>>>> On 21 April 2014 19:44, Matt Sicker <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>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 <[email protected]>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Matt Sicker <[email protected]>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Matt Sicker <[email protected]>
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Matt Sicker <[email protected]>
>>>>
>>>
>>>
>>>
>>> --
>>>
>>> Bruce Brouwer
>>>
>>
>>
>>
>> --
>> Matt Sicker <[email protected]>
>>
>>
>
>
> --
> Matt Sicker <[email protected]>
>
--
Bruce Brouwer
