Yeah, I'm looking into the CC failures today, there does seem to be a proliferation of failures (almost 4% at the moment). -- Manik Surtani Lead, JBoss Cache JBoss, a division of Red Hat Email: manik(a)jboss.org Telephone: +44 7786 702 706 MSN: manik(a)surtani.org Yahoo/AIM/Skype: maniksurtani On 8 Dec 2006, at 04:36, Brian Stansberry wrote: > 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