[hibernate-dev] Bytecode enhanced, Reference Cached immutable Entities

[Steve Ebersole]

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 <sanne at 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 <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
>