[hibernate-dev] [ORM] Synchronization on AbstractLoadPlanBasedLoader

[Sanne Grinovero]

On 14 December 2015 at 17:10, Vlad Mihalcea <mihalcea.vlad at gmail.com> 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 at hibernate.org>
> 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 at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
>