11:32 a.m.
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(a)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(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).