Hello, thanks for your work! I don't have much to comment except that; - we use tabs for indentation - logs are a bit verbose but I can work on that when applying the patch (will be faster then explaining everything) - are you sure you can statically reference ConcurrentStatisticsImpl wo failing at runtime on all VMs? I don't thing the JLS guarantees that. You might have to load the class via reflection - don't use Class.forName, prefer ReflectHelper.classForName - anybody knows what svn:executable means? - in ConcurrentQueryStatisticsImpl I don't quite understand your read/write lock use. They look backwards. Also the weird for loops in execute are probably worth a comment or two - use StringBuilder in toString for the concurrent impls
Alex, can you sign the JBoss contributor agreement (somewhat similar tot he Apache foundation one) https://www.jboss.org/contribute and ping steve afterwards. I've also commented inline your questions. On 14 déc. 2009, at 12:13, Alex Snaps wrote: > Attached is my current diff against trunk r18223 for you guys to review: > - it introduces org.hibernate.stat.Concurrent* classes that are the JDK5 ones > - I extracted interfaces for the Collection-, Entity-, Query- and > SecondLevelCacheStatistics and have their previous impl. in *Impl. > (the new are Concurrent*Impl) > - SessionFactory's constructor tries to load AtomicLong and on > availability uses the new concurrent stats, otherwise falls back to > the previous impl. > - Dirty reads are still in the old impl., Yet I'd at least consider > the StatisticsImpl.isStatisticsEnabled to not be dirty-read and mark > it as volatile, wdyt? volatile in 1.4 is meaningless so why bother. > - As for your HashMap's and the infinite loop trap, I need to look > into the 1.4 impl. again, the unsynchronized Map.keySet() might be an > issue indeed > > All the code is Java 1.4 compliant (I removed the generics or > covariant return types I had in there) > imho, the impact is not disruptive, but I'll wait for your input to be > definitive on this :) > Don't hesitate to ask for some changes, especially if these help > getting this backported to prior 3.x releases! > Waiting on your input, and hope I can then submit the final patch on jira > Thanks, > Alex > > On Wed, Dec 9, 2009 at 4:58 PM, Steve Ebersole <st...@hibernate.org> wrote: >> I'm actually ok with just dropping 1.4 support for 3.5. >> >> If you are going to do it, I'd say that "dirty read" here is ok. What >> I'd worry about though is concurrency issues like blocking (reads on >> unsynchronized maps can cause blocks iirc). >> >> As for backporting, well it depends, on a few things. First, if you >> want it backported to version prior to 3.5 it will absolutely need some >> form of 1.4 compatibility. I think backport-util-concurrent, as you >> mention, is the best option. Then its mostly a matter of how disruptive >> the change is. >> >> On Wed, 2009-12-09 at 14:12 +0100, Alex Snaps wrote: >>> What about the dirty reads? Should I mark all getters as synchronized >>> methods, or just leave it as is? Not knowing how intensively these >>> getters are being called... >>> Btw, would this be backported into older 3.x releases as well? >>> >>> On Wed, Dec 9, 2009 at 1:39 PM, Emmanuel Bernard <emman...@hibernate.org> >>> wrote: >>>> I don't think it's critical to backport this for 1.4 JDK users. But if you >>>> want to spare cycles... >>>> >>>> On 9 déc. 2009, at 12:48, Alex Snaps wrote: >>>> >>>>> I have finished a first version of it all: >>>>> It is supporting both jdk 1.4 and 1.5+. So that if the >>>>> java.util.concurrent classes are present, it will use the new >>>>> ConcurrentStatisticsImpl, otherwise will fallback to the current >>>>> StatisticsImpl. As mentioned, I had to extract interfaces for >>>>> EntityStatistics, CollectionStatistics, SecondLevelCacheStatistics and >>>>> QueryStatistics. >>>>> Now, there is still the issue of the dirty reads within the current >>>>> StatisticsImpl. (no synchronization on read)... >>>>> What would you guys think of fixing these with the >>>>> backport-util-concurrent ? So that even the 1.4 jdk get better >>>>> concurrency on these... >>>>> That would include a new dependency... I'm also currently looking into >>>>> not including the deps, and using the same tricks as they are doing to >>>>> diminish contention... >>>>> Their AtomicLong relies on synchronization, but at least every stat >>>>> gets its own lock, versus everyone competing for the same one as it is >>>>> currently the case. >>>>> wdyt? >>>>> >>>>> On Tue, Dec 1, 2009 at 3:59 PM, Steve Ebersole <st...@hibernate.org> >>>>> wrote: >>>>>> I guess I have just been waiting until we can actually leverage 1.5 >>>>>> features (ala utilize enums or expose generics/typing). That will not >>>>>> happen for 3.5. >>>>>> >>>>>> Now statistics are encapsulated behind a set of interfaces (Statistics >>>>>> and StatisticsImplementor). We could make this alterable like I did for >>>>>> JDBC 3/4 based on the JVM. That would mean reflection code though. >>>>>> >>>>>> I do not actually know of any real cases of Hiberate being used in 1.4 >>>>>> environments today. So maybe we can just make it 1.5 compatible. >>>>>> >>>>>> Votes? >>>>>> >>>>>> >>>>>> On Tue, 2009-12-01 at 14:38 +0100, Alex Snaps wrote: >>>>>>> We've been doing some improvement to the Hibernate statistics at >>>>>>> Terracotta, when we realized how much the synchronization on it was >>>>>>> impacting throughput in our tests. >>>>>>> That is work we wanted to contribute back to you guys, should >>>>>>> Hibernate Core be target at 1.5. As it seems that's not yet the case, >>>>>>> so there isn't much you guys will be able to do with these changes... >>>>>>> We discussed about that at Devoxx with Max and Emmanuel and thought it >>>>>>> was okay to have 1.5 impl. of the specs (java.util.concurrent based) >>>>>>> already. Apparently not :( What timeframe do you see 1.4 support being >>>>>>> dropped? >>>>>>> >>>>>>> On Tue, Dec 1, 2009 at 1:44 PM, Steve Ebersole <st...@hibernate.org> >>>>>>> wrote: >>>>>>>> I have issues reloading Maven-based projects in IntelliJ as well. I >>>>>>>> simply try to minimize the number of times I reload. >>>>>>>> >>>>>>>> Hibernate is *built* with JDK 1.5, but not all the modules are 1.5 >>>>>>>> compatible. >>>>>>>> >>>>>>>> What "statistics work" discussion? I must have missed that. But for >>>>>>>> sure the hibernate-core module should remain 1.4 compatible. Dropping >>>>>>>> 1.4 support is on the roadmap, but not for 3.5 >>>>>>>> >>>>>>>> >>>>>>>> On Tue, 2009-12-01 at 11:32 +0100, Alex Snaps wrote: >>>>>>>>> Hey, >>>>>>>>> Doing a svn update of the Hibernate trunk, I realized I probably had >>>>>>>>> changed the project to be Java5 manually as it reverted to 1.4 >>>>>>>>> (because of some pom.xml change) in IntelliJ. >>>>>>>>> Talking to Max and Emmanuel at Devoxx I thought trunk was now to be >>>>>>>>> Java 5? Is this not the case after all, or are poms only update when >>>>>>>>> the first Java5 language/jdk feature sneaks in? >>>>>>>>> As discussed we discussed, all the statistics work heavily rely on >>>>>>>>> java.util.concurrent classes, so that is "more or less" important for >>>>>>>>> that patch... >>>>>>>>> Btw do you guys have a contributor agreement somewhere, I couldn't >>>>>>>>> find it. >>>>>>>>> Thanks, >>>>>>>>> Alex >>>>>>>>> >>>>>>>> -- >>>>>>>> Steve Ebersole <st...@hibernate.org> >>>>>>>> Hibernate.org >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>> -- >>>>>> Steve Ebersole <st...@hibernate.org> >>>>>> Hibernate.org >>>>>> >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> Alex Snaps <alex.sn...@gmail.com> >>>>> Software Engineer - Terracotta >>>>> http://twitter.com/alexsnaps >>>>> http://www.linkedin.com/in/alexsnaps >>>>> >>>>> _______________________________________________ >>>>> hibernate-dev mailing list >>>>> hibernate-dev@lists.jboss.org >>>>> https://lists.jboss.org/mailman/listinfo/hibernate-dev >>>> >>>> >>> >>> >>> >> -- >> Steve Ebersole <st...@hibernate.org> >> Hibernate.org >> >> > > > > -- > Alex Snaps <alex.sn...@gmail.com> > Software Engineer - Terracotta > http://twitter.com/alexsnaps > http://www.linkedin.com/in/alexsnaps > <HibernateStats.diff> _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
