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

[John O'Hara]

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


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