[hibernate-dev] Bytecode enhanced, Reference Cached immutable Entities
John O'Hara
johara at redhat.com
Fri Mar 27 09:11:38 EDT 2015
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 at 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 at 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 at 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 at 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 at hibernate.org>
>>>>> wrote:
>>>>>>
>>>>>> On Thu, Mar 26, 2015 at 10:20 AM, Sanne Grinovero <sanne at 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 at 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 at lists.jboss.org
>>>>>>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>>>>>
>>>>
>>>>
>>>> --
>>>> John O'Hara
>>>> johara at 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 at 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 at 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).
More information about the hibernate-dev
mailing list