On Tuesday, 22 April 2014, Bruce Brouwer <[email protected]> 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" <[email protected]> > 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 <[email protected]> > 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 <[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 < > > -- Matt Sicker <[email protected]>
