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
