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 <[email protected]> wrote: > > > 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]> > -- Bruce Brouwer
