Hi all, while reviewing an improvement by Stale about reducing synchronization, I'm having the impression that the synchronization could be completely removed. But there's a comment warning me against that, so for sake of safety I'm merging the improvement without risking going too far: // synchronized to avoid multi-thread access issues; defined as method synch to avoid // potential deadlock issues due to nature of code. I tried to figure what "potential deadlock" it's referring to, but I'm having the impression the comment might be outdated. So I've reduced the contention to the only include the code block about which I'm not confident. By looking into git history, it seems the comment isn't related to any specific fix but was included already when this class was first created. Would someone be able to point out what is the issue this is protecting against? That should allow us to provide an even better patch, although I'll apply the safe one for now so to at least have the benefits already when wrapping of result-sets is disabled. thanks, Sanne _______________________________________________ hibernate-dev mailing list hibernate-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev

On 14 December 2015 at 17:10, Vlad Mihalcea <mihalcea.vlad(a)gmail.com&gt; wrote: > Hi, > > We really need to test it thoroughly because the current pooled optimizer > are reasonably fast especially when used with a database sequence. > The table generators are slow because of the row-level locking, so I won't > include those in this discussion. > > What strikes me is the synchronized block which might cause contention we > didn't have before. Yes, but that's actually a sign I'm very happy with ;-) The performance team from Red Hat has been helping a lot the past few years, and Hibernate 5 incorporates many improvements based on both their patches and their regular feedback. They are now able to scale it up like never before, and so doing these synchronization points are now "bottlenecks", while they would previously not be because of other things slowing it down. > I'd also vote for a new optimizer instead of modifying the pooled or the > pooled-lo ones. > The current optimizer are quite easy to grasp, and, if we are to add a > high-performance one, I think a new implementation is better suited for this > task. Maybe, if we're considering the new ones experimental, but these changes are relatively simple and aren't changing the fundamental design. I am not sure how far it helps end users to have a long list of choices, when to understand which one to use one has to describe the implementation in detail. I would rather use some short-hand names which describe the different requirements one might have - for example someone might need monotonically increasing sequences, while someone else might be happy with non-strictly monotonical sequences if they can get better performance out of it. >From the different names, we should be able to activate the right implementations; for example the choice between an highly optimized implementation which would only work for non-multitenant use cases like Steve suggested could be an implementation detail: we'd opt to use that implementation internally when someone picks "pooled-lo" and happens to not be using multitenancy. But of course power users are always right, so if someone configures an implementation explicitly by naming it by fully qualified classname, then we should of course assume that the user knows best (although not as much as to not validate - for example again - that he's using a non-multitenant one with multitenancy). Thanks, Sanne > > Vlad > > On Mon, Dec 14, 2015 at 6:25 PM, Sanne Grinovero <sanne(a)hibernate.org&gt; > wrote: >> >> Hi all, >> while reviewing an improvement by Stale about reducing >> synchronization, I'm having the impression that the synchronization >> could be completely removed. >> >> But there's a comment warning me against that, so for sake of safety >> I'm merging the improvement without risking going too far: >> >> // synchronized to avoid multi-thread access issues; defined as >> method synch to avoid >> // potential deadlock issues due to nature of code. >> >> I tried to figure what "potential deadlock" it's referring to, but I'm >> having the impression the comment might be outdated. So I've reduced >> the contention to the only include the code block about which I'm not >> confident. >> By looking into git history, it seems the comment isn't related to any >> specific fix but was included already when this class was first >> created. >> >> Would someone be able to point out what is the issue this is protecting >> against? >> >> That should allow us to provide an even better patch, although I'll >> apply the safe one for now so to at least have the benefits already >> when wrapping of result-sets is disabled. >> >> thanks, >> Sanne >> _______________________________________________ >> hibernate-dev mailing list >> hibernate-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/hibernate-dev > > _______________________________________________ hibernate-dev mailing list hibernate-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev

I'm confused. Sanne was talking about a completely different piece of code from optimizers. Maybe you are mixing this and the other current hibernate-dev discussion? On Mon, Dec 14, 2015 at 11:10 AM Vlad Mihalcea <mihalcea.vlad(a)gmail.com&gt; wrote: > Hi, > > We really need to test it thoroughly because the current pooled optimizer > are reasonably fast especially when used with a database sequence. > The table generators are slow because of the row-level locking, so I won't > include those in this discussion. > > What strikes me is the synchronized block which might cause contention we > didn't have before. > I'd also vote for a new optimizer instead of modifying the pooled or the > pooled-lo ones. > The current optimizer are quite easy to grasp, and, if we are to add a > high-performance one, I think a new implementation is better suited for > this task. > > Vlad > > On Mon, Dec 14, 2015 at 6:25 PM, Sanne Grinovero <sanne(a)hibernate.org&gt; > wrote: > > > Hi all, > > while reviewing an improvement by Stale about reducing > > synchronization, I'm having the impression that the synchronization > > could be completely removed. > > > > But there's a comment warning me against that, so for sake of safety > > I'm merging the improvement without risking going too far: > > > > // synchronized to avoid multi-thread access issues; defined as > > method synch to avoid > > // potential deadlock issues due to nature of code. > > > > I tried to figure what "potential deadlock" it's referring to, but I'm > > having the impression the comment might be outdated. So I've reduced > > the contention to the only include the code block about which I'm not > > confident. > > By looking into git history, it seems the comment isn't related to any > > specific fix but was included already when this class was first > > created. > > > > Would someone be able to point out what is the issue this is protecting > > against? > > > > That should allow us to provide an even better patch, although I'll > > apply the safe one for now so to at least have the benefits already > > when wrapping of result-sets is disabled. > > > > thanks, > > Sanne > > _______________________________________________ > > hibernate-dev mailing list > > hibernate-dev(a)lists.jboss.org > > https://lists.jboss.org/mailman/listinfo/hibernate-dev > > > _______________________________________________ > hibernate-dev mailing list > hibernate-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev >

It's very possible that code comments may no longer be pertinent.

On Mon, Dec 14, 2015 at 10:26 AM Sanne Grinovero <sanne(a)hibernate.org&gt; wrote: > > Hi all, > while reviewing an improvement by Stale about reducing > synchronization, I'm having the impression that the synchronization > could be completely removed. > > But there's a comment warning me against that, so for sake of safety > I'm merging the improvement without risking going too far: > > // synchronized to avoid multi-thread access issues; defined as > method synch to avoid > // potential deadlock issues due to nature of code. > > I tried to figure what "potential deadlock" it's referring to, but I'm > having the impression the comment might be outdated. So I've reduced > the contention to the only include the code block about which I'm not > confident. > By looking into git history, it seems the comment isn't related to any > specific fix but was included already when this class was first > created. > > Would someone be able to point out what is the issue this is protecting > against? > > That should allow us to provide an even better patch, although I'll > apply the safe one for now so to at least have the benefits already > when wrapping of result-sets is disabled. > > thanks, > Sanne > _______________________________________________ > hibernate-dev mailing list > hibernate-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev

I did not add those comments; they were just in some code I copied over into that class. On Tue, Dec 15, 2015, 4:02 AM Sanne Grinovero <sanne(a)hibernate.org&gt; wrote: > On 15 December 2015 at 01:46, Steve Ebersole <steve(a)hibernate.org&gt; wrote: > > It's very possible that code comments may no longer be pertinent. > > Right, that's what I'm trying to figure out. Do you remember which > possible deadlock it might have referred to? > > > > > On Mon, Dec 14, 2015 at 10:26 AM Sanne Grinovero <sanne(a)hibernate.org&gt; > > wrote: > >> > >> Hi all, > >> while reviewing an improvement by Stale about reducing > >> synchronization, I'm having the impression that the synchronization > >> could be completely removed. > >> > >> But there's a comment warning me against that, so for sake of safety > >> I'm merging the improvement without risking going too far: > >> > >> // synchronized to avoid multi-thread access issues; defined as > >> method synch to avoid > >> // potential deadlock issues due to nature of code. > >> > >> I tried to figure what "potential deadlock" it's referring to, but I'm > >> having the impression the comment might be outdated. So I've reduced > >> the contention to the only include the code block about which I'm not > >> confident. > >> By looking into git history, it seems the comment isn't related to any > >> specific fix but was included already when this class was first > >> created. > >> > >> Would someone be able to point out what is the issue this is protecting > >> against? > >> > >> That should allow us to provide an even better patch, although I'll > >> apply the safe one for now so to at least have the benefits already > >> when wrapping of result-sets is disabled. > >> > >> thanks, > >> Sanne > >> _______________________________________________ > >> hibernate-dev mailing list > >> hibernate-dev(a)lists.jboss.org > >> https://lists.jboss.org/mailman/listinfo/hibernate-dev > _______________________________________________ hibernate-dev mailing list hibernate-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev