On 26/03/15 15:20, Sanne Grinovero 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. The thought process behind having a central factory was to have a single place to make the determination which EntityEntry implementation to instantiate.
I can see your point below, it would mean that we would have to make the determination as to which factory to use somewhere else in the code base. > > 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 "" 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) > - .. Initially, I was thinking shared between sessions, and a EntityEntry that contained a mutable state, but I agree that "ImmutableEntityEntry" and "MutableEntityEntry" make more logical sense. Had not considered what properties could be removed in the ImmutableEntityEntry implementation, but will take a detailed look at what could be removed for that use case. > > 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. AFAIK there shouldn't be any need to promote an ImmutableEntityEntry to a MutableEntityEntry, we should know when the Entity is loaded that it is Immutable and I can not think why that would change during the lifetime of a session. > > 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() ? This was an area that I was unsure as to what the best property(s) to test were for mutability. I can see now that checking if the Entity was loaded READ_ONLY might not be appropriate as the Entity might be mutable. > > 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. I am unsure of the relationship between EntityPersister, EntityEntry and a Session. Are you proposing that you determine the implementation of EntityEntry and cache a reference in EntityPersister so that we can re-use across sessions? Would we need some type checking to ensure that the EntryEntity implementation is correct for each creation of EntityEntry, and throw something like a InvalidEntityEntryException if the EntityEntry we are creating is not appropriate? As I said, I am not sure of the relationships so do not know what assumptions can be made around this. > 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. +1 had not considered this in my draft. will ensure that the ImmutableEntityEntry is threadsafe. > > 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. I have not considered LockMode, not sure of the implications of LockMode and what impact it might have. Any guidance here would be greatly appreciated. > > 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. An oversight on my part, will move the javadoc back to the public api, I will move EntityEntryExtraState to different package > 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? +1 > 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. Do we need to think about the different use cases (default, immutable reference cached, ORM) to define a generic API that fits all? > > 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. >> -- John O'Hara joh...@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). _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
