The only distinction we care about is EntityPetsister#isMutable. The
read-only case should be handled by the normal MutableEntityEntry. In
other words there is nothing to pass to EntityPersister to make a decision
based on; just getEntityEntryBuilder ().
As far as LockMode, I think it makes the most sense to not do locking for
immutable entities. Not sure if no-op or exception is the best way to
handle. But regardless either way means we do not account for locking for
the immutable - entity EntityEntry.
Which brings up another point. We should decide whether the
mutable/immutable in these names refers to the fact that the entity is
mutable/immutable or refers to the state of the EntityEntry itself. I
think it is best to agree on that semantic now before we potentially start
using these in situations they were not designed for.
On Mar 27, 2015 8:11 AM, "John O'Hara" <johara(a)redhat.com> wrote:
On 27/03/15 12:14, Sanne Grinovero wrote:
> Also from OGM and Search perspective, we're not expecting any of this
> to be backported.
> It's not a functional blocker, "just" significant performance
> improvements.
>
> How do we move further? John, I'm not familiar with the requirements
> for the bytecode instrumentation, but can help a bit on the other
> aspects. I could either work on extracting the patch of converting the
> EntityEntry into an interface from your draft, or if you have time to
> polish that I could review that PR. After that change is integrated,
> it should be easier for us to work in parallel on different areas and
> draft some ideas with the factory context idea.
>
I had started to work on extracting just the EntityEntryInterface and
factory, so can continue on that. Which issue do we want to log this under.
There is already HHH-9265?
> +1 to an EntityPersister:EntityEntryFactory 1:1 relation, glad to see
> it makes sense in your minds as well.
>
> To make a decision on the Immutable entities vs Read Only entities..
> are we good in applying the optimisation only to Immutable entities or
> do we need to find a way for entities which happen to be loaded
> read-only too? I might have an idea to play with for that, but it
> obviously gets a bit more complex.
>
I think for our case that we only need Immutable entities, not sure of any
other scenarios where you would need to optimise Read Only entities
>
> @Steve, I guess we're looking at you for advice on what needs to be
> done for the Lock state. I'm understanding that needs to be addressed
> right?
>
> Thanks,
> Sanne
>
>
>
>
> On 27 March 2015 at 11:42, John O'Hara <johara(a)redhat.com> wrote:
>
>> Steve,
>>
>> From our point of view, we would not be looking to backport any features
>> introduced to any of the ORM4 releases. Our focus has shifted to the next
>> major release of EAP as we have a number of issues we need to overcome.
>> Getting this feature into ORM5 sooner rather than later is essential for
>> our
>> use case in the future.
>>
>> Thanks
>>
>> John
>>
>>
>> On 27/03/15 11:36, Steve Ebersole wrote:
>>
>> BTW, if we go this route this *cannot* be backported because we do allow
>> users to override EntityPersister; its an extension point. Adding a new
>> method to extension points in bugfixes is a no-no.
>>
>> On Fri, Mar 27, 2015 at 6:35 AM, Steve Ebersole <steve(a)hibernate.org>
>> wrote:
>>
>>> An EntityPersister describes each entity you defined in your model. Its
>>> one-to-one between @Entity and EntityPersister. And really, it is
>>> totally
>>> reasonable to have the EntityPersister determine the "EntityEntry
>>> builder"
>>> to use. That also eliminates needing to develop
>>> yet-another-extension-point
>>> because if we expose it from EntityPersister OGM already swaps
>>> EntityPersister impls so this would JustWork.
>>>
>>> On Fri, Mar 27, 2015 at 6:04 AM, John O'Hara <johara(a)redhat.com>
wrote:
>>>
>>>> On 26/03/15 16:22, Steve Ebersole wrote:
>>>>
>>>> The contract I suggested btw is missing one use case I know we have
>>>> discussed in regards to OGM a few times... the ability to store
>>>> references
>>>> to datastore specific locators (I think the use case was to
>>>> efficiently load
>>>> collections through that datastore reference). But that (obviously)
>>>> requires passing extra, specific state into the createEntityEntry
>>>> method.
>>>> Not really sure the best way to handle such things tbh.
>>>>
>>>> If we did have a "creation Context" parameter passed to the
>>>> createEntityEntry, we would be instantiating more objects, however if
>>>> we
>>>> went with Sanne' suggestion if caching the EnityEntryFactory with
the
>>>> EntityPersister, how many objects would be actually created? I am not
>>>> sure
>>>> about the relationship between Entity, EntityEntry and Session but
>>>> could
>>>> this a possibility if there is a 1-1 relationship between
>>>> EntityPersister
>>>> and no. of different Entities definitions?
>>>>
>>>>
>>>> On Thu, Mar 26, 2015 at 11:16 AM, Steve Ebersole
<steve(a)hibernate.org>
>>>> wrote:
>>>>
>>>>> Apparently I hit some key combo that means send :) To finish up...
>>>>>
>>>>> On Thu, Mar 26, 2015 at 10:58 AM, Steve Ebersole
<steve(a)hibernate.org
>>>>> >
>>>>> wrote:
>>>>>
>>>>>>
>>>>>> On Thu, Mar 26, 2015 at 10:20 AM, Sanne Grinovero <
>>>>>> sanne(a)hibernate.org>
>>>>>> wrote:
>>>>>>
>>>>>>>
>>>>>>> Concurrency
>>>>>>> Since the goal is to share the ImmutableEntityEntry
instance
>>>>>>> among
>>>>>>> multiple threads reading it, I'd rather make sure that it
is really
>>>>>>> immutable. For example it now holds onto potentially lazy
>>>>>>> initialized
>>>>>>> fields, such as getEntityKey().
>>>>>>> If it's not possible to make it really immutable (all
final fields),
>>>>>>> we'll need to make it threadsafe and question the name
I'm
>>>>>>> proposing.
>>>>>>>
>>>>>>
>>>>>> The specific use-case John is interested in does indeed need to
be
>>>>>> completely thread-safe and fully concurrent.
>>>>>>
>>>>>> LockMode
>>>>>>> From a logical perspective of users, one might think that
an
>>>>>>> entity
>>>>>>> being "immutable" doesn't necessarily imply I
can't lock on it..
>>>>>>> right? I'm not sure how far it could be a valid use case,
but these
>>>>>>> patches are implying that one can't lock an immutable
entity
>>>>>>> anymore,
>>>>>>> as the lock state would be as immutable as anything else on
the
>>>>>>> EntityEntry.
>>>>>>> Are we good with that? Alternatively one might need to think
to
>>>>>>> separate the lock state handling from the EntityEntry.
>>>>>>>
>>>>>>
>>>>>> Conceptually there is nothing wrong with requesting a READ lock
on an
>>>>>> immutable entity. But like you said, what is the logical
>>>>>> expectation there?
>>>>>> IMO there should be none. But if we decide that it is OK to req
>>>>>>
>>>>>
>>>>> But if we decide that it is OK to request a lock on an immutable
>>>>> entity,
>>>>> that is problematic for the idea of a "SharedEntityEntry"
or an
>>>>> "ImmutableEntityEntry" because a lock is associated with a
Session
>>>>> which is
>>>>> what the EntityEntry is meant to model... an entity's information
in
>>>>> relation to a Session. Aka, it now needs to hold state.
>>>>>
>>>>>
>>>>> Notice I said immutable here and not READ_ONLY which is a specific
>>>>>> distinction which is important to other parts of your email that
I
>>>>>> will
>>>>>> address in a second email.
>>>>>>
>>>>>>
>>>>>>
>>>>>>> On smaller details:
>>>>>>> # org.hibernate.engine.internal.SharedEntityEntry is hosted
in an
>>>>>>> .internal package, I don't think it's right to
refactor all the
>>>>>>> public
>>>>>>> API javadoc which was referring to EntityEntry to now refer
to the
>>>>>>> internal implementation.
>>>>>>> # things like EntityEntryExtraState should probably get moved
to
>>>>>>> .internal packages as well now - we couldn't do that
before without
>>>>>>> breaking either encapsulation or APIs.
>>>>>>>
>>>>>>
>>>>> +1
>>>>>
>>>>>
>>>>>>> In terms of git patches, the complexity of the changeset
risks to
>>>>>>> get
>>>>>>> a bit our of hand. What about we focus on creating a clean
pull
>>>>>>> request which focuses exclusively on making EntityEntry an
>>>>>>> interface,
>>>>>>> and move things to the right packages and javadoc?
>>>>>>>
>>>>>>
>>>>> Agree 100%
>>>>>
>>>>> You'd have a trivial EntitEntryFactory, and we can then build
the
>>>>>>> evolution on top of that, not least maybe helping out by
challenging
>>>>>>> some points in parallel work.
>>>>>>> These are the things I'd leave for a second iteration:
>>>>>>> - add various implementations of EntityEntry iteratively,
as
>>>>>>> needed
>>>>>>> - the strategy such a Factory would use the pick an
implementation
>>>>>>> - ultimately, make it possible for an integrator to
override such
>>>>>>> a
>>>>>>> Factory
>>>>>>>
>>>>>>
>>>>> This all seems reasonable. For 5.0 I think the most important
>>>>> thing is
>>>>> to nail down the idea of EntityEntry as an interface, introduce a
>>>>> Factory
>>>>> for building them and agree on the signature for building them.
>>>>> Granted we
>>>>> may need iteration to finalize the actual Factory signature,
>>>>> especially as
>>>>> OGM finally starts to use it, but I think that in general we can get
>>>>> it
>>>>> pretty close. Worst case we just pass high-level constructs like
the
>>>>> EntityPersister (which OGM supplies custom impls) and the Session
and
>>>>> all
>>>>> the "EntityEntry state". For the purpose of starting a
discussion:
>>>>>
>>>>> public interface EntityEntryFactory {
>>>>> public EntityEntry createEntityEntry(
>>>>> PersistenceContext persistenceContext,
>>>>> EntityPersister persister,
>>>>> Status status,
>>>>> Serializable id,
>>>>> Object version,
>>>>> Object[] loadedState,
>>>>> Object rowId,
>>>>> LockMode lockMode,
>>>>> boolean existsInDatabase,
>>>>> boolean disableVersionIncrement,
>>>>> boolean lazyPropertiesAreUnfetched);
>>>>> }
>>>>>
>>>>> I would suggest a "creation context" method param object to
pass in
>>>>> here, but seeing as how we are trying to stop instantiations...
>>>>>
>>>>> I would prefer the definition of the EntityEntryFactory to use be
>>>>> defined via SessionFactoryBuilder interface. I already have plans
to
>>>>> have a
>>>>> auto-loaded hook for integrators to be able to adjust the
>>>>> SessionFactoryBuilder. But as a Service is fine too.
>>>>>
>>>>> For example with Hibernate OGM we might want to override / re
>>>>>>> configure the factories to use custom EntityEntry
implementations -
>>>>>>> requirements are not fully clear at this point but it seems
likely.
>>>>>>>
>>>>>>> The priority being to define the API as that would be a
blocker for
>>>>>>> 5.0, we have then better choices to leave more smarter and
advanced
>>>>>>> EntityEntry implementations for the future; we'd still
need to
>>>>>>> implement at least the essential ones to make sure the API of
the
>>>>>>> EntityEntryFactory has all the context it needs.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Sanne
>>>>>>>
>>>>>>>
>>>>>>> On 24 March 2015 at 09:27, John O'Hara
<johara(a)redhat.com> wrote:
>>>>>>>
>>>>>>>> Steve,
>>>>>>>>
>>>>>>>> Have you had chance to look at this? Do you have any
>>>>>>>> comments/observations?
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>>> John
>>>>>>>>
>>>>>>>>
>>>>>>>> On 17/03/15 09:24, John O'Hara wrote:
>>>>>>>>
>>>>>>>> Steve,
>>>>>>>>
>>>>>>>> I have been having a think about the EntityEntry
interface, and
>>>>>>>> have
>>>>>>>> forked
>>>>>>>> a branch here:
>>>>>>>>
>>>>>>>>
>>>>>>>>
https://github.com/johnaoahra80/hibernate-orm/
>>>>>>>> tree/EntityEntryInterface
>>>>>>>>
>>>>>>>> I know it is nowhere near complete, but was this the sort
of idea
>>>>>>>> you had in
>>>>>>>> mind?
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>>> John
>>>>>>>>
>>>>>>>>
>>>>>>>> On 13/03/15 09:44, John O'Hara wrote:
>>>>>>>>
>>>>>>>> EntityEntry retains a reference to a persistenceContext
internally
>>>>>>>> that
>>>>>>>> org.hibernate.engine.spi.EntityEntry#setReadOnly makes
calls to,
>>>>>>>> is
>>>>>>>> this
>>>>>>>> where the session reference is kept? As
>>>>>>>> org.hibernate.engine.spi.PersistenceConext is an
interface could
>>>>>>>> we
>>>>>>>> have a
>>>>>>>> different implementation for this use case? e.g. an
>>>>>>>> ImmutablePersistenceContext that could be shared across
sessions?
>>>>>>>>
>>>>>>>> For the bytecode enhancement, could we change the
enhancer so that
>>>>>>>> it adds
>>>>>>>> an EntityEntry interface with javassist.
>>>>>>>> ClassPool.javassist.ClassPool.makeInterface()() as
opposed to
>>>>>>>> adding
>>>>>>>> a class
>>>>>>>> javassist.ClassPool.makeClass()? I need to have a look at
javassit
>>>>>>>> to
>>>>>>>> confirm what javassist.ClassPool.makeInterface() does.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>>> John
>>>>>>>>
>>>>>>>> On 12/03/15 18:52, Steve Ebersole wrote:
>>>>>>>>
>>>>>>>> It is possible. Although some of the changes are
particularly
>>>>>>>> painful.
>>>>>>>> Most of EntityEntry, if it is an interface, can be made
to work
>>>>>>>> with
>>>>>>>> your
>>>>>>>> use case.
org.hibernate.engine.spi.EntityEntry#setReadOnly I
>>>>>>>> think
>>>>>>>> is the
>>>>>>>> one exception, because:
>>>>>>>> 1) your use case needs it
>>>>>>>> 2) it expects the Session to be available internally (its
not
>>>>>>>> passed)
>>>>>>>>
>>>>>>>> The bigger thing I am worried about for you is the
bytecode stuff,
>>>>>>>> as that
>>>>>>>> ties very tightly with EntityEntry.
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>> hibernate-dev mailing list
>>>>>>> hibernate-dev(a)lists.jboss.org
>>>>>>>
https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>> --
>>>> John O'Hara
>>>> johara(a)redhat.com
>>>>
>>>> JBoss, by Red Hat
>>>> Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
>>>> Street, Windsor, Berkshire, SI4 1TE, United Kingdom.
>>>> Registered in UK and Wales under Company Registration No. 3798903
>>>> Directors: Michael Cunningham (USA), Charlie Peters (USA), Matt Parsons
>>>> (USA) and Michael O'Neill (Ireland).
>>>>
>>>>
>>
>> --
>> John O'Hara
>> johara(a)redhat.com
>>
>> JBoss, by Red Hat
>> Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
>> Street,
>> Windsor, Berkshire, SI4 1TE, United Kingdom.
>> Registered in UK and Wales under Company Registration No. 3798903
>> Directors:
>> Michael Cunningham (USA), Charlie Peters (USA), Matt Parsons (USA) and
>> Michael O'Neill (Ireland).
>>
>>
--
John O'Hara
johara(a)redhat.com
JBoss, by Red Hat
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SI4 1TE, United Kingdom.
Registered in UK and Wales under Company Registration No. 3798903
Directors: Michael Cunningham (USA), Charlie Peters (USA), Matt Parsons
(USA) and Michael O'Neill (Ireland).