Re: [hibernate-dev] Pooled Optimiser Improvements

[Sanne Grinovero]

I love Steve's idea of having a dedicated implementation for
non-multitenant users.

But +1 to also apply this improvement.

Sanne

On 14 December 2015 at 01:44, Stuart Douglas  wrote:
>
>
> - Original Message -
>> From: "Scott Marlow" 
>> To: "Stuart Douglas" , hibernate-dev@lists.jboss.org
>> Sent: Friday, 11 December, 2015 10:54:15 PM
>> Subject: Re: [hibernate-dev] Pooled Optimiser Improvements
>>
>> Should this be a specialized pooled optimizer that is only used in
>> environments that do not suffer from leaving the ThreadLocal around
>> after the application is undeployed?  In other words, the expectation is
>> that classloader leaks with this pooled optimizer are expected (e.g.
>> user must restart the jvm to really undeploy the application completely).
>>
>> I am thinking that there are at least three typical situations:
>>
>> 1.  Applications are deployed in Java standalone edition.  Generally,
>> when the app undeploys the jvm is shutting down.
>
> In this case it is a non-issue, as the JVM will be destroyed on shutdown.
>
>>
>> 2. Applications are deployed as part of some container (e.g. an EE
>> server) and the Hibernate jars are on the global classloader path (or
>> something like that).  On each shared container thread, there would be
>> one Optimizer for all deployed applications.  I wonder if instead, we
>> would want one Optimizer instance per Hibernate SessionFactory
>> associated with the many container threads?
>
> AFAIK there is one optimiser per table, not one global optimiser. On undeploy 
> these Optimisers will no longer be referenced and should be eligible for GC, 
> which will clean up the thread local.
>
> It is importable to note that this is not a static thread local, which are 
> very prone to leaks.
>
>>
>> 3. Applications are deployed as part of some container (e.g. an EE
>> server) and the Hibernate jars are deployed with the application. The
>> ThreadLocals are associated with threads that are shared by different
>> deployed applications. The application classloader contains the
>> Hibernate classes. Each deployed application has its own Optimizer
>> threadlocal. On each shared container thread, there would be one
>> Optimizer per application (since each application has its Optimizer TL).
>>   Like (2), there would be sharing of the same Optimizer with the many
>> application session factories.  Should we instead have an optimizer per
>> session factory?
>
> The same this applied here, once the application is undeployed the 
> ThreadLocal should be eligible for GC.
>
> Stuart
>
>>
>> Scott
>>
>> On 12/10/2015 11:31 PM, Stuart Douglas wrote:
>> > Hello,
>> >
>> > I have been working on a change to the pooled optimizer that we have been
>> > seeing good performance results with. Basically it hands out blocks of
>> > ID's to a thread local, rather than having every thread contend on the
>> > lock every time an ID is required.
>> >
>> > https://github.com/hibernate/hibernate-orm/compare/master...stuartwdouglas:pooled-optimiser-hack
>> >
>> > What would I need to do to get a change like this in? In particular:
>> >
>> > - Does it need to be a new type of optimizer, or is modifying the existing
>> > one like I have done OK?
>> > - How should it be configured?
>> >
>> > I am happy to do up a PR for this, but I am just not really sure what would
>> > be required to get it to a point where it would be acceptable for
>> > inclusion.
>> >
>> > Stuart
>> > ___
>> > hibernate-dev mailing list
>> > hibernate-dev@lists.jboss.org
>> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
>> >
>>
> ___
> hibernate-dev mailing list
> hibernate-dev@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev

[hibernate-dev] [ORM] Synchronization on AbstractLoadPlanBasedLoader

[Sanne Grinovero]

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@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev

Re: [hibernate-dev] [ORM] Synchronization on AbstractLoadPlanBasedLoader

[Vlad Mihalcea]

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 
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@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev

[hibernate-dev] [OGM] The (new) remote Hot-Rod based Infinispan GridDialect

[Sanne Grinovero]

Hello all,
   while creating the basic scaffolding for the new GridDialect, I
called the new Maven module "hibernate-ogm-infinispan-hotrod". Which
is rather long, but descriptive.

Q1: any better name?


   The current one which we have working on Infinispan "embedded mode"
is named "hibernate-ogm-infinispan".

Q2: do we need to rename the existing one? If not, what to we call it
in our documentation to disambiguate?


Thanks,
Sanne
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev

Re: [hibernate-dev] [OGM] The (new) remote Hot-Rod based Infinispan GridDialect

[Davide D'Alto]

I would use:
hibernate-ogm-infinispan-hotrod

For Neo4j we have a similart problem
at the moment the directory is hibernate-ogm-neo4j and the short name is
'neo4j_embedded'
Whatever we choose I guess we should be consistent:


I guess haivng
- hibernate-ogm-infinispan/embedded
- hibernate-ogm-infinispan/hotrod

On Mon, Dec 14, 2015 at 5:57 PM, Sanne Grinovero 
wrote:

> Hello all,
>while creating the basic scaffolding for the new GridDialect, I
> called the new Maven module "hibernate-ogm-infinispan-hotrod". Which
> is rather long, but descriptive.
>
> Q1: any better name?
>
>
>The current one which we have working on Infinispan "embedded mode"
> is named "hibernate-ogm-infinispan".
>
> Q2: do we need to rename the existing one? If not, what to we call it
> in our documentation to disambiguate?
>
>
> Thanks,
> Sanne
> ___
> hibernate-dev mailing list
> hibernate-dev@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev

Re: [hibernate-dev] [OGM] The (new) remote Hot-Rod based Infinispan GridDialect

[Gunnar Morling]

Does it have to be a separate module to begin with?

For MongoDB - which contains two datastore providers (MongoDB, Fongo)
and Redis - which also will have two different dialects as per Mark's
pending PR - it's one module.

We should stick to one pattern, and having one module seems easier on
the user to me. So unless you see a strong advantage for two modules
I'd say let's use one.

Regarding the provider names, "infinispan" and "infinspan-remote" seem
good. If you think remote will be more common eventually, we may
rename the current one and have "infinispan-embedded" and
"infinispan". Requires a change to existing users, but it seems
acceptable to do in 5.

I would not have "hotrod" in the name, this is a technicality I'd
prefer to not expose at this level. Rather "remote" vs. "embedded"
which will be stable also if specific protocols change.

--Gunnar


2015-12-14 18:57 GMT+01:00 Sanne Grinovero :
> Hello all,
>while creating the basic scaffolding for the new GridDialect, I
> called the new Maven module "hibernate-ogm-infinispan-hotrod". Which
> is rather long, but descriptive.
>
> Q1: any better name?
>
>
>The current one which we have working on Infinispan "embedded mode"
> is named "hibernate-ogm-infinispan".
>
> Q2: do we need to rename the existing one? If not, what to we call it
> in our documentation to disambiguate?
>
>
> Thanks,
> Sanne
> ___
> hibernate-dev mailing list
> hibernate-dev@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev

Re: [hibernate-dev] [OGM] The (new) remote Hot-Rod based Infinispan GridDialect

[Davide D'Alto]

Sorry for the previous email, I've sent it too soon pressing some strange
keywords combination.

> We should stick to one pattern, and having one module seems easier on
> the user to me. So unless you see a strong advantage for two modules
> I'd say let's use one.

One general disadvantage I can see is that you add in the classpath
dependencies that you don't need (maybe).

> I would not have "hotrod" in the name, this is a technicality I'd
> prefer to not expose at this level. Rather "remote" vs. "embedded"

Are there other backends that could be associated with infinispan? Maybe a
different remote protocol?

> Regarding the provider names, "infinispan" and "infinspan-remote" seem
> good. If you think remote will be more common eventually, we may
> rename the current one and have "infinispan-embedded" and
> "infinispan". Requires a change to existing users, but it seems
> acceptable to do in 5.

I think that in general, remote dbs are more common. I would prefer
"infinispan-embedded" and "infinispan".

I guess based on what we decide we should also adapt the names and the
modules for Neo4j to be consistent.


On Mon, Dec 14, 2015 at 6:09 PM, Gunnar Morling 
wrote:

> Does it have to be a separate module to begin with?
>
> For MongoDB - which contains two datastore providers (MongoDB, Fongo)
> and Redis - which also will have two different dialects as per Mark's
> pending PR - it's one module.
>
> We should stick to one pattern, and having one module seems easier on
> the user to me. So unless you see a strong advantage for two modules
> I'd say let's use one.
>
> Regarding the provider names, "infinispan" and "infinspan-remote" seem
> good. If you think remote will be more common eventually, we may
> rename the current one and have "infinispan-embedded" and
> "infinispan". Requires a change to existing users, but it seems
> acceptable to do in 5.
>
> I would not have "hotrod" in the name, this is a technicality I'd
> prefer to not expose at this level. Rather "remote" vs. "embedded"
> which will be stable also if specific protocols change.
>
> --Gunnar
>
>
> 2015-12-14 18:57 GMT+01:00 Sanne Grinovero :
> > Hello all,
> >while creating the basic scaffolding for the new GridDialect, I
> > called the new Maven module "hibernate-ogm-infinispan-hotrod". Which
> > is rather long, but descriptive.
> >
> > Q1: any better name?
> >
> >
> >The current one which we have working on Infinispan "embedded mode"
> > is named "hibernate-ogm-infinispan".
> >
> > Q2: do we need to rename the existing one? If not, what to we call it
> > in our documentation to disambiguate?
> >
> >
> > Thanks,
> > Sanne
> > ___
> > hibernate-dev mailing list
> > hibernate-dev@lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
> ___
> hibernate-dev mailing list
> hibernate-dev@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev

Re: [hibernate-dev] [OGM] The (new) remote Hot-Rod based Infinispan GridDialect

[Sanne Grinovero]

On 14 December 2015 at 18:09, Gunnar Morling  wrote:
> Does it have to be a separate module to begin with?
>
> For MongoDB - which contains two datastore providers (MongoDB, Fongo)
> and Redis - which also will have two different dialects as per Mark's
> pending PR - it's one module.
>
> We should stick to one pattern, and having one module seems easier on
> the user to me. So unless you see a strong advantage for two modules
> I'd say let's use one.

The two are very different; in terms of end-user concerns the
featuresets are different; for example one is able to integrate with
JTA transactions, the other isn't. One requires Externalizers, the
other requires protobuf schemas, etc.. even the configuration methods
and available options will be totally different to end users.

The dependencies are extremely different too; you might consider these
"an implementation detail", yet the Hot Rod client requires a lot more
stuff which I'd rather not push to users of Infinispan Embedded.

To have Infinispan Embedded users avoid this mess, they'd either need
to depend on a different artefact or to deal with lists of exclusions.
I think having a different artefact is nicer.

So, be it to clarify differences in featuresets. architecture or how
to configure it all, in terms of documentation I think we should
clearly introduce the split right after introducing what Infinispan
is, and there is were I need some proper terminology to be
established.

>
> Regarding the provider names, "infinispan" and "infinspan-remote" seem
> good. If you think remote will be more common eventually, we may
> rename the current one and have "infinispan-embedded" and
> "infinispan". Requires a change to existing users, but it seems
> acceptable to do in 5.

Right but if we already are suspecting that this might change, I'd
rather do it now:
 - we're preparing a major release
 - will impact more users later (hopefully growing userbase?)

> I would not have "hotrod" in the name, this is a technicality I'd
> prefer to not expose at this level. Rather "remote" vs. "embedded"
> which will be stable also if specific protocols change.

Good point, I'm ok to go with "Infinispan Remote" if you think that's
easier to new users, but that's inconsistent with the choice of the
Infinispan project; also keep in mind we're coupling this new dialect
to Hot Rod specifically.
Also "Hot Rod" is the brand name of Infinispan remoting technology; if
it evolves, it will evolve the protocol but it's unlikely to change
its name.

Sanne


>
> --Gunnar
>
>
> 2015-12-14 18:57 GMT+01:00 Sanne Grinovero :
>> Hello all,
>>while creating the basic scaffolding for the new GridDialect, I
>> called the new Maven module "hibernate-ogm-infinispan-hotrod". Which
>> is rather long, but descriptive.
>>
>> Q1: any better name?
>>
>>
>>The current one which we have working on Infinispan "embedded mode"
>> is named "hibernate-ogm-infinispan".
>>
>> Q2: do we need to rename the existing one? If not, what to we call it
>> in our documentation to disambiguate?
>>
>>
>> Thanks,
>> Sanne
>> ___
>> hibernate-dev mailing list
>> hibernate-dev@lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev

Re: [hibernate-dev] [ORM] Synchronization on AbstractLoadPlanBasedLoader

[Sanne Grinovero]

On 14 December 2015 at 17:10, Vlad Mihalcea  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 
> 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@lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
>
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev

Re: [hibernate-dev] [ORM] Synchronization on AbstractLoadPlanBasedLoader

[Steve Ebersole]

Sanne, you just described the general approach of "short naming" for config
options overall :)



On Mon, Dec 14, 2015 at 6:28 PM Sanne Grinovero  wrote:

> On 14 December 2015 at 17:10, Vlad Mihalcea 
> 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 
> > 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@lists.jboss.org
> >> https://lists.jboss.org/mailman/listinfo/hibernate-dev
> >
> >
> ___
> hibernate-dev mailing list
> hibernate-dev@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev

Re: [hibernate-dev] Pooled Optimiser Improvements

[Ståle W Pedersen]

Hi, we've done some runs now with 5.0.6-SNAPSHOT (built today) and we're 
seeing a lot more contention compared to the same build without Stuart's 
patch.
If possible I would like to see this being merged, but if it's not 
possible Steve's suggestion should also work afaik.

ståle

On 14.12.15 15:12, Sanne Grinovero wrote:
>I love Steve's idea of having a dedicated implementation for
>non-multitenant users.
>
>But +1 to also apply this improvement.
>
>Sanne
>
>On 14 December 2015 at 01:44, Stuart Douglas  wrote:
>>
>>
>> - Original Message -
>>> From: "Scott Marlow" 
>>> To: "Stuart Douglas" , hibernate-dev@lists.jboss.org
>>> Sent: Friday, 11 December, 2015 10:54:15 PM
>>> Subject: Re: [hibernate-dev] Pooled Optimiser Improvements
>>>
>>> Should this be a specialized pooled optimizer that is only used in
>>> environments that do not suffer from leaving the ThreadLocal around
>>> after the application is undeployed?  In other words, the expectation is
>>> that classloader leaks with this pooled optimizer are expected (e.g.
>>> user must restart the jvm to really undeploy the application completely).
>>>
>>> I am thinking that there are at least three typical situations:
>>>
>>> 1.  Applications are deployed in Java standalone edition.  Generally,
>>> when the app undeploys the jvm is shutting down.
>>
>> In this case it is a non-issue, as the JVM will be destroyed on shutdown.
>>
>>>
>>> 2. Applications are deployed as part of some container (e.g. an EE
>>> server) and the Hibernate jars are on the global classloader path (or
>>> something like that).  On each shared container thread, there would be
>>> one Optimizer for all deployed applications.  I wonder if instead, we
>>> would want one Optimizer instance per Hibernate SessionFactory
>>> associated with the many container threads?
>>
>> AFAIK there is one optimiser per table, not one global optimiser. On 
>> undeploy these Optimisers will no longer be referenced and should be 
>> eligible for GC, which will clean up the thread local.
>>
>> It is importable to note that this is not a static thread local, which are 
>> very prone to leaks.
>>
>>>
>>> 3. Applications are deployed as part of some container (e.g. an EE
>>> server) and the Hibernate jars are deployed with the application. The
>>> ThreadLocals are associated with threads that are shared by different
>>> deployed applications. The application classloader contains the
>>> Hibernate classes. Each deployed application has its own Optimizer
>>> threadlocal. On each shared container thread, there would be one
>>> Optimizer per application (since each application has its Optimizer TL).
>>>   Like (2), there would be sharing of the same Optimizer with the many
>>> application session factories.  Should we instead have an optimizer per
>>> session factory?
>>
>> The same this applied here, once the application is undeployed the 
>> ThreadLocal should be eligible for GC.
>>
>> Stuart
>>
>>>
>>> Scott
>>>
>>> On 12/10/2015 11:31 PM, Stuart Douglas wrote:
>>> > Hello,
>>> >
>>> > I have been working on a change to the pooled optimizer that we have been
>>> > seeing good performance results with. Basically it hands out blocks of
>>> > ID's to a thread local, rather than having every thread contend on the
>>> > lock every time an ID is required.
>>> >
>>> > https://github.com/hibernate/hibernate-orm/compare/master...stuartwdouglas:pooled-optimiser-hack
>>> >
>>> > What would I need to do to get a change like this in? In particular:
>>> >
>>> > - Does it need to be a new type of optimizer, or is modifying the existing
>>> > one like I have done OK?
>>> > - How should it be configured?
>>> >
>>> > I am happy to do up a PR for this, but I am just not really sure what 
>>> > would
>>> > be required to get it to a point where it would be acceptable for
>>> > inclusion.
>>> >
>>> > Stuart
>>> > ___
>>> > hibernate-dev mailing list
>>> > hibernate-dev@lists.jboss.org
>>> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>> >
>>>
>> ___
>> hibernate-dev mailing list
>> hibernate-dev@lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>___
>hibernate-dev mailing list
>hibernate-dev@lists.jboss.org
>https://lists.jboss.org/mailman/listinfo/hibernate-dev
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev

Re: [hibernate-dev] [ORM] Synchronization on AbstractLoadPlanBasedLoader

[Steve Ebersole]

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

On Mon, Dec 14, 2015 at 10:26 AM Sanne Grinovero 
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@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev

Re: [hibernate-dev] [ORM] Synchronization on AbstractLoadPlanBasedLoader

[Steve Ebersole]

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 
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 
> 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@lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
> >
> ___
> hibernate-dev mailing list
> hibernate-dev@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev

Re: [hibernate-dev] Pooled Optimiser Improvements

[Scott Marlow]

On 12/13/2015 08:44 PM, Stuart Douglas wrote:
>
>
> - Original Message -
>> From: "Scott Marlow" 
>> To: "Stuart Douglas" , hibernate-dev@lists.jboss.org
>> Sent: Friday, 11 December, 2015 10:54:15 PM
>> Subject: Re: [hibernate-dev] Pooled Optimiser Improvements
>>
>> Should this be a specialized pooled optimizer that is only used in
>> environments that do not suffer from leaving the ThreadLocal around
>> after the application is undeployed?  In other words, the expectation is
>> that classloader leaks with this pooled optimizer are expected (e.g.
>> user must restart the jvm to really undeploy the application completely).
>>
>> I am thinking that there are at least three typical situations:
>>
>> 1.  Applications are deployed in Java standalone edition.  Generally,
>> when the app undeploys the jvm is shutting down.
>
> In this case it is a non-issue, as the JVM will be destroyed on shutdown.
>
>>
>> 2. Applications are deployed as part of some container (e.g. an EE
>> server) and the Hibernate jars are on the global classloader path (or
>> something like that).  On each shared container thread, there would be
>> one Optimizer for all deployed applications.  I wonder if instead, we
>> would want one Optimizer instance per Hibernate SessionFactory
>> associated with the many container threads?
>
> AFAIK there is one optimiser per table, not one global optimiser. On undeploy 
> these Optimisers will no longer be referenced and should be eligible for GC, 
> which will clean up the thread local.

Good point.  Looks like its associated with each mapped entity class 
(which is per Hibernate session factory).

>
> It is importable to note that this is not a static thread local, which are 
> very prone to leaks.

Good point, so no leak.

>
>>
>> 3. Applications are deployed as part of some container (e.g. an EE
>> server) and the Hibernate jars are deployed with the application. The
>> ThreadLocals are associated with threads that are shared by different
>> deployed applications. The application classloader contains the
>> Hibernate classes. Each deployed application has its own Optimizer
>> threadlocal. On each shared container thread, there would be one
>> Optimizer per application (since each application has its Optimizer TL).
>>Like (2), there would be sharing of the same Optimizer with the many
>> application session factories.  Should we instead have an optimizer per
>> session factory?
>
> The same this applied here, once the application is undeployed the 
> ThreadLocal should be eligible for GC.
>
> Stuart
>
>>
>> Scott
>>
>> On 12/10/2015 11:31 PM, Stuart Douglas wrote:
>>> Hello,
>>>
>>> I have been working on a change to the pooled optimizer that we have been
>>> seeing good performance results with. Basically it hands out blocks of
>>> ID's to a thread local, rather than having every thread contend on the
>>> lock every time an ID is required.
>>>
>>> https://github.com/hibernate/hibernate-orm/compare/master...stuartwdouglas:pooled-optimiser-hack
>>>
>>> What would I need to do to get a change like this in? In particular:
>>>
>>> - Does it need to be a new type of optimizer, or is modifying the existing
>>> one like I have done OK?
>>> - How should it be configured?
>>>
>>> I am happy to do up a PR for this, but I am just not really sure what would
>>> be required to get it to a point where it would be acceptable for
>>> inclusion.
>>>
>>> Stuart
>>> ___
>>> hibernate-dev mailing list
>>> hibernate-dev@lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>>
>>
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev

Re: [hibernate-dev] Pooled Optimiser Improvements

[Scott Marlow]

On 12/11/2015 09:30 AM, Steve Ebersole wrote:
> It's hard to say without understanding the scenario where you are seeing
> this as a problem.  I have some guesses as to what may be the problem,
> but without understanding more about why you see this as a problem in
> the first place it is hard to give you an answer.  For example, I wonder
> if for environments not using multi-tenancy whether the recent changes
> for the generators to support multi-tenancy might be the culprit.  If
> that is the case, and those changes are in fact the underlying cause of
> the perf issues you see then I think there is actually a better
> solution.  But again, its hard to say unless we understand the reason
> this "shows up" as a perf problem for you.

As best as I can tell from looking at the current PooledLoOptimizer, 
versus the proposed change (to have a chunk of ids per thread), we went 
from accessing a contented lock, to instead using per thread memory 
(eliminating the contended lock on PooledLoOptimizer.generate()).

>
> Until we hear more I think at this stage I'd vote for a separate
> optimizer.  And maybe even not one that is upstream.
>
> Also I agree with Scott that I am VERY leery of not cleaning up a
> ThreadLocal.

My mistake, as Stuart pointed out, the TL is not static, so we shouldn't 
introduce any leaks.

>
> On Fri, Dec 11, 2015 at 7:55 AM Scott Marlow  > wrote:
>
> Should this be a specialized pooled optimizer that is only used in
> environments that do not suffer from leaving the ThreadLocal around
> after the application is undeployed?  In other words, the expectation is
> that classloader leaks with this pooled optimizer are expected (e.g.
> user must restart the jvm to really undeploy the application
> completely).
>
> I am thinking that there are at least three typical situations:
>
> 1.  Applications are deployed in Java standalone edition.  Generally,
> when the app undeploys the jvm is shutting down.
>
> 2. Applications are deployed as part of some container (e.g. an EE
> server) and the Hibernate jars are on the global classloader path (or
> something like that).  On each shared container thread, there would be
> one Optimizer for all deployed applications.  I wonder if instead, we
> would want one Optimizer instance per Hibernate SessionFactory
> associated with the many container threads?
>
> 3. Applications are deployed as part of some container (e.g. an EE
> server) and the Hibernate jars are deployed with the application. The
> ThreadLocals are associated with threads that are shared by different
> deployed applications. The application classloader contains the
> Hibernate classes. Each deployed application has its own Optimizer
> threadlocal. On each shared container thread, there would be one
> Optimizer per application (since each application has its Optimizer TL).
>Like (2), there would be sharing of the same Optimizer with the many
> application session factories.  Should we instead have an optimizer per
> session factory?
>
> Scott
>
> On 12/10/2015 11:31 PM, Stuart Douglas wrote:
>  > Hello,
>  >
>  > I have been working on a change to the pooled optimizer that we
> have been seeing good performance results with. Basically it hands
> out blocks of ID's to a thread local, rather than having every
> thread contend on the lock every time an ID is required.
>  >
>  >
> 
> https://github.com/hibernate/hibernate-orm/compare/master...stuartwdouglas:pooled-optimiser-hack
>  >
>  > What would I need to do to get a change like this in? In particular:
>  >
>  > - Does it need to be a new type of optimizer, or is modifying the
> existing one like I have done OK?
>  > - How should it be configured?
>  >
>  > I am happy to do up a PR for this, but I am just not really sure
> what would be required to get it to a point where it would be
> acceptable for inclusion.
>  >
>  > Stuart
>  > ___
>  > hibernate-dev mailing list
>  > hibernate-dev@lists.jboss.org 
>  > https://lists.jboss.org/mailman/listinfo/hibernate-dev
>  >
> ___
> hibernate-dev mailing list
> hibernate-dev@lists.jboss.org 
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev

Re: [hibernate-dev] [ORM] Synchronization on AbstractLoadPlanBasedLoader

[Vlad Mihalcea]

Hi,

Sorry for the confusion, I mistakenly replied on a different thread.

Vlad

On Tue, Dec 15, 2015 at 3:48 AM, Steve Ebersole  wrote:

> 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 
> 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 
>> 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@lists.jboss.org
>> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
>> >
>> ___
>> hibernate-dev mailing list
>> hibernate-dev@lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>
>
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev