[hibernate-dev] Hibernate & Java 5 ?

Alex Snaps alex.snaps at gmail.com
Tue Dec 15 05:26:15 EST 2009 

Comments inline:

On Tue, Dec 15, 2009 at 10:46 AM, Emmanuel Bernard
<emmanuel at hibernate.org> wrote:
> Hello, thanks for your work!
>
> I don't have much to comment except that;
>  - we use tabs for indentation

My fault, should have looked up your code style first! Will fix for
the final patch. Are they somewhere available for dwl for IntelliJ?

>  - logs are a bit verbose but I can work on that when applying the patch (will be faster then explaining everything)

Cool, thanks.

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

I'll look into those two

>  - anybody knows what svn:executable means?

heck if I would! set from our svn commit style I guess :P

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

Well they aren't, but I should probably explain these two things in
the code indeed. I'll already do so here and add some comment in the
code about these:
The RRWL is not there for visibility, but to provide an atomic view on
getExecutionAvgTime(). Indeed, by acquiring the write lock I can make
sure no other thread is currently in the middle of executing the
executed(long, long) method, so that get getExecutionAvgTime() always
returns correct result. executed(long, long) though, can now be
executed concurrently by multiple threads. The RRWL though raises the
cost of reading that statistic, as all updating threads would have to
wait for the release of the write lock to acquire the read one
again...
As for the weird loop, it raises the chances for a spin wait, vs. a
context switch in case of the compareAndSet fails, which would cost
less to the Thread executing

>  - use StringBuilder in toString for the concurrent impls

I'll fix that

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

To my understanding it is not, but my understanding of the 1.4 JMM is
also limited.
It suffers the reordering issue, but we don't care about this here.
imho, marking it as volatile would have its value always synched with
main memory. At least that way we're sure all threads do see stats are
either enabled or not (vs. than eventually or even worse maybe). The
synchronized methods should take care of the rest. I'll double check
this statement though and will get back to you...

>
>> - 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 <steve at 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 <emmanuel at 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 <steve at 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 <steve at 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 <steve at hibernate.org>
>>>>>>>>> Hibernate.org
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>> --
>>>>>>> Steve Ebersole <steve at hibernate.org>
>>>>>>> Hibernate.org
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Alex Snaps <alex.snaps at gmail.com>
>>>>>> Software Engineer - Terracotta
>>>>>> http://twitter.com/alexsnaps
>>>>>> http://www.linkedin.com/in/alexsnaps
>>>>>>
>>>>>> _______________________________________________
>>>>>> hibernate-dev mailing list
>>>>>> hibernate-dev at lists.jboss.org
>>>>>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>> --
>>> Steve Ebersole <steve at hibernate.org>
>>> Hibernate.org
>>>
>>>
>>
>>
>>
>> --
>> Alex Snaps <alex.snaps at gmail.com>
>> Software Engineer - Terracotta
>> http://twitter.com/alexsnaps
>> http://www.linkedin.com/in/alexsnaps
>> <HibernateStats.diff>
>
>



-- 
Alex Snaps <alex.snaps at gmail.com>
Software Engineer - Terracotta
http://twitter.com/alexsnaps
http://www.linkedin.com/in/alexsnaps

More information about the hibernate-dev mailing list