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.

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.

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. 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? 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 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&gt; 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

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&gt; wrote: > > On Thu, Mar 26, 2015 at 10:20 AM, Sanne Grinovero <sanne(a)hibernate.org&gt; > 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&gt; 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 >> > >

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&gt; 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&gt; > wrote: > >> >> On Thu, Mar 26, 2015 at 10:20 AM, Sanne Grinovero <sanne(a)hibernate.org&gt; >> 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&gt; 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&#39;Harajohara(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).

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 <mailto:steve@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 <mailto:johara@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 <mailto:steve@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 <mailto:steve@hibernate.org>> wrote: > > > On Thu, Mar 26, 2015 at 10:20 AM, Sanne Grinovero > <sanne(a)hibernate.org <mailto:sanne@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 <mailto:johara@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 > <mailto:hibernate-dev@lists.jboss.org> > https://lists.jboss.org/mailman/listinfo/hibernate-dev > > > > -- John O'Hara johara(a)redhat.com <mailto:johara@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).

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.

+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.

@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&gt; 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&gt; 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&gt; 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&gt; >>> 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&gt; >>>> wrote: >>>>> >>>>> On Thu, Mar 26, 2015 at 10:20 AM, Sanne Grinovero <sanne(a)hibernate.org&gt; >>>>> 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&gt; 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). >

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.

Now the "meat"... On Thu, Mar 26, 2015 at 10:20 AM, Sanne Grinovero <sanne(a)hibernate.org <mailto:sanne@hibernate.org>> wrote: What is your primary differentiator between 'SharedEntityEntry' and 'StatefulEntityEntry' ? For our purposes I'd have used different names, but since there's no javadoc yet I wonder if you had different intentions. Personally I'd have chosen something like "ImmutableEntityEntry" and "MutableEntityEntry", there the Mutable one is a rename of the existing implementation, and the Immutable would be a slimmed down version which might not need fields such as: - loadedState (not needed for readonly) - version (what would be the point) - .. Well I cannot speak for John's reasoning, but knowing his use case I think that what you describe is exactly his intent with the split. SharedEntityEntry represents an EntityEntry that is immutable and needs to be because it is *shared* between Sessions. I personally prefer the names mutable and immutable as well. The docs for ImmutableEntityEntry should make it clear that it is shared between Sessions and that concurrency is the reason it is immutable.

A concern I have is to avoid ever needing to "promote" an ImmutableEntityEntry into a MutableEntityEntry: it's easy to mark an existing instance of ImmutableEntityEntry as READ_ONLY, but there is no going back if the entity entry was initially loaded as READ_ONLY. One could think of swapping the existing entityentry, but that could get hairy and defeats the point of optimising object allocations. Is there a strong guarantee which we can rely on, that if an EntityEntry is marked READ_ONLY at first load, noone will ever need to re-mark it as mutable? If not, the current check in DefaultEntityEntryFactory basing the choice on the current status of the Entity might not be enough, we might need to be a bit more conservative and only based that on getPersister().isMutable() ? So there is a distinction here between an immutable entity (EntityPersister#isMutable == false) and a entity loaded as read-only specific to a Session. The first kind can *never* become non-read-only. The second kind can. The first kind should be the only case where we use ImmutableEntityEntry, because in the second case that entity can later become mutable and as you say we would then need to "promote" its corresponding ImmutableEntityEntry to a MutableEntityEntry (even more instantiations!).