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" <[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 <[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 > > >
