Re: [jbosscache-dev] RE: JBCACHE-874 (WAS: JBCACHE-315 and 1.4.1)

This change is made on 1.4 and HEAD. When I looked at it, the effiency gain from using a Set vs a List only exists with the locks collection. So that's the only one I changed to LinkedHashSet. The methods that add things to the other list fields (e.g. modifications) don't do a contains() check to see if what they are adding is already there, so they don't have the inefficiency. Presumably this means that with normal behavior the same object wouldn't get added twice to these lists. A cursory check showed that to be the case, and if it weren't I'd expect we'd have noticeable bugs. So, I decided not to change to other fields to LinkedHashSet -- didn't want to introduce issues by making a change. At least not in 1.4. I ran the testsuite and saw no regressions vs cc. But, there are a lot of failures now on cc, so I'm going to wait to close JBCACHE-874 until we get some cleaner test runs. jbosscache-dev-bounces(a)lists.jboss.org wrote: > Manik Surtani wrote: >> On 5 Dec 2006, at 19:29, Brian Stansberry wrote: >> >>> Manik Surtani wrote: >>>> Brian Stansberry wrote: >>>>> It's not a big deal if JBCACHE-874 doesn't go in 1.4.1.CR1, >>>>> but it >>>>> would be nice to get it in so it's out in the wild longer before >>>>> the GA. >>>> >>>> Sorry yes, forgot about this. >>>> >>> >>> If you're delaying 1.4.1.CR1 for this, let me know, because at this >>> point I was deferring it a bit pending anything that comes out of >>> this thread. >> >> What would you like me to do? When do you think you could have 874 >> in? >> > > Probably by tomorrow. If there's going to be a CR2 I'd prefer > you don't delay CR1 for this. Getting things out in the wild > is good, the benefit of having this out in CR1 is outweighed > by the benefit of getting everything else in CR1 out in the > wild sooner. > >>> >>>>> >>>>> 1) A bunch of the LinkedList fields in TransactionEntry are >>>>> protected. Changing them to LinkedHashSet therefore technically >>>>> changes the API. This is an internal class, so I don't think >>>>> that's an issue. Any objections? >>>> >>>> I'm ok with this, I don't think it will break the >>>> OptimisticTransactionEntry subclass. In fact, is there any reason >>>> why this needs to be protected? Could we make do with stronger >>>> access protection than protected? >>>> >>> >>> I don't see any reason why not; this is an internal class. I much >>> prefer private fields; hate protected 'cause it's never clear to me >>> if the class designer had a strong reason to expose them or just >>> did >>> it "in case some subclass wants it some day". IMHO a protected >>> getter/setter with a private field is a much better indication of >>> design intent and allows clean subsequent addition of things like >>> lazy initialization. >> >> +1. And like I said, if we can make do with package-level access >> for >> the getter/setter and private for the field, I'd go with that if it >> makes sense. >> > > I'll check; we may not even need the getters/setters for this > particular case. I don't think the subclass uses any of the > protected fields. And if not, I don't think there is any real > "design intent" to expose them. > If later on, we find a real need to expose them, then we add them. > >>> >>>>> >>>>> 2) A number of TransactionEntry methods return List and currently >>>>> hand out a ref to the internal data structure. The caller of >>>>> these methods logically expects a List so I think the API should >>>>> remain the same. So, that implies the methods would do something >>>>> like "return new ArrayList(internalSet);" >>>>> >>>> >>>> Hmm, I guess so. Perhaps in 2.0.0, change this to return a >>>> Collection? >>>> >>> >>> Maybe. Depends on whether callers are relying on an expected >>> ordering. I had a quick look at these methods yesterday. At least >>> one is never invoked and thus can be dropped. For another, at least >>> in some cases, Set semantics are fine, so a possibility for >>> 1.4.1 is >>> an added method that exposes the internal Set. >> >> Ok, I'm happy with this. Like you said, this *is* an internal class >> anyway. >> > > Yes. And if there is a path whereby outside code can access > this class (I think there is), adding methods doesn't break > any compatibility. > >>> >>> Another possibility for 2.0.0 is to return an Iterator over the >>> internal field, which would give the same ordering as a List >>> with no >>> extra effort. I expect all the callers already end up iterating >>> over the List anyway. >> >> Is this the case? What about calling contains() on the List? I >> thought I saw this somewhere ... > > You did, but the contains() call is done internally to > TransactionEntry whenever any of the add methods are called > -- in order to ensure Set semantics on the internal lists. > Getting rid of this inefficiency is the primary benefit of > JBCACHE-874. > > - Brian > > _______________________________________________ > jbosscache-dev mailing list > jbosscache-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/jbosscache-dev