Thanks for your thoughts Sanne. I am going to break my responses down into digestable chunks to follow...
On Thu, Mar 26, 2015 at 10:20 AM, Sanne Grinovero <sa...@hibernate.org> wrote: > [adding the mailing list] > > Generally speaking, looks like we agree on the direction: EntityEntry > needs to be an interface, and some clever logic to select the > appropriate implementations. > In your draft you're having a single EntityEntryFactory as a global > service; I'm wondering if we shouldn't have the possibiliy to have a > different factory implementation per Entry type.. more on this below. > > 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) > - .. > > 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() ? > > The READ_ONLY point which you're leveraging for this specific > optimisation seems to be key for the specific optimisation we have in > mind at this point; but generalizing the concept it seems to me that > the choice of EE implementation to use for a specific Entity type will > be a consistent choice for the lifecycle of the EntityPersister, and > depending on immutable flags on the EntityPersister. Which is why I'm > suggesting that the EntityPersister should have a dedicated > EntityEntryFactory. Making the EntityEntryFactory a global service > would force to go through all the checks of the EE implementation > choice each time, while the choice should always be the same. I > wouldn't argue to save a couple of simple "if" evaluations, but it's > very possible that some more clever EntityEntryFactory implementations > than this current draft might need to do more work, for example > consult more Services to call back into OGM metadata. > Not least, having a per-type EntityEntryFactory would make it possible > to refer to it from some EntityEntry implementations and save some > memory around the common state. > > 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 <joh...@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@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
