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]>
