[hibernate-dev] HHH-9701 - Develop "immutable EntityEntry" impl

Fri Apr 17 17:12:35 EDT 2015

And change the name of DefaultEntityEntryFactory :)

On Fri, Apr 17, 2015 at 2:04 PM, John O'Hara <johara at redhat.com> wrote:

>  Thanks for reviewing, I was unsure of the best way to test if the Entity
> was referenced cached (or could be cached), I look at
> org.hibernate.persister.entity.EntityPersister#canUseReferenceCacheEntries
> instead.
>
> Would it matter if the entity was Immutable and not cached? In that case
> we would not be nulling the ref to the EntityEntry, but the Entity could
> not be shared between sessions. Would the Entity object (and EntityEntry
> object) be GC'd after a session was closed if the entity was not cached? Is
> there the potential for a mem leak there?
>
> I will also change the Exception name and cache the EntityEntryFactory
> when the EntityPersister is constructed
>
>
>
>
> On 17/04/15 19:55, Steve Ebersole wrote:
>
> I commented on your pull request.
>
>  My main concern is how you determine "canClearEntityEntryReference".
> That needs to change.  See my comment.
>
>
>
> On Mon, Apr 13, 2015 at 5:37 AM, John O'Hara <johara at redhat.com> wrote:
>
>>  I think there are two factors to consider, when the entity is marked as
>> Immutable, and when the EntityEntry can be shared between sessions.
>>
>> Our use case concerns the ImmutableEntityEntry being cached and shared
>> between sessions, so we need to check if the Entity is reference cached in
>> the 2lc.
>>
>> Therefore I think the answer to the question below is;
>>
>>
>>    - I would consider the the trigger for using ImmutableEntityEntry
>>    when the Entity is marked as Immutable;
>>    - I would consider the trigger for not nulling the EntityEntry in the
>>    EntityEntryContext when the EntityEntry is an ImmutableEntityEntry and the
>>    Entity is reference cached in the 2lc
>>
>>
>> There might be further optimizations for ImmutableEntityEntries that are
>> not cached and shared between sessions that I am not aware of.
>>
>> It seems the most logical place to determine whether to create a
>> MutableEntityEntry or an ImmutableEntityEntry is when a call is made to
>> AbstractEntityPersister getEntityEntryFactory() and we can test to see if
>> the Entity is Immutable.
>>
>> I was unsure how of the best method to determine if the Enity would be
>> cached in the 2lc, so tested at the point EntityEntry is nulled in
>> EntityEntryContext. removeEntityEntry() or EntityEntryContext.clear()
>>
>>
>>
>> On 10/04/15 17:46, Steve Ebersole wrote:
>>
>> To post my question again...
>>
>>  It really depends on what y'all consider the trigger for using
>> ImmutableEntityEntry.  When would the EntityPersister use the
>> EntityEntryFactory producing ImmutableEntityEntry instances?
>> a) when the entity is marked immutable?
>> b) when the entity is marked immutable *and* we need to cache it by
>> reference?
>> c) some other condition?
>>
>>  Because the answer to this ^^ dictates how we need to check whether we
>> can clear this reference
>>
>>
>> On Fri, Apr 10, 2015 at 11:45 AM, Steve Ebersole <steve at hibernate.org>
>> wrote:
>>
>>> Well the performance is exactly what I had in mind when I asked you
>>> about the exact "trigger for using ImmutableEntityEntry" in your
>>> use-case.  But you never replied to that.
>>>
>>>
>>>
>>> On Fri, Apr 10, 2015 at 9:26 AM, John O'Hara <johara at redhat.com> wrote:
>>>
>>>>  I have updated my current branch:
>>>> https://github.com/johnaoahra80/hibernate-orm/tree/HHH-9701
>>>>
>>>>    - Extracted the deserialization of EntityEntry in
>>>>    EntityEntryContext into a separate method:
>>>>    https://github.com/johnaoahra80/hibernate-orm/commit/d9f5dc76e05a111d37dbdac64c6aa1bd485a1ba9
>>>>    - Created a LockModeException for invalid lock modes for an
>>>>    ImmutableEntity:
>>>>    https://github.com/johnaoahra80/hibernate-orm/commit/99e308ae267f9bfc30547784087955d7dc1e73e0
>>>>
>>>> Do we need a new Exception type here, or could we just use a
>>>> HibernateException with a message about invalid lock type?
>>>>
>>>>    - Added a check when EntityEntryContext.clear() or EntityEntry.
>>>>    removeEntityEntry() is called to see if the EntityEntry is
>>>>    ImmutableEntityEntry AND the Entity is referenced cached in the 2lc:
>>>>    https://github.com/johnaoahra80/hibernate-orm/commit/310a2eb5b8988189806a2e688090a7ea16ae5474
>>>>
>>>> Not sure of the performance implications on this, I am planning a run
>>>> on WF9 to test and regressions in our use case.
>>>>
>>>>    - Extracted an AbstractEntityEntry superclass that
>>>>    MutableEntityEntry and ImmutableEntityEntry both inherit from:
>>>>    https://github.com/johnaoahra80/hibernate-orm/commit/df50d344441ed4cc8f3c3c8df90edc106dd52adb
>>>>
>>>>  John
>>>>
>>>>
>>>>
>>>>
>>>> On 07/04/15 20:20, Steve Ebersole wrote:
>>>>
>>>> TBH, I have no idea what happens to comments on a Pull Request when you
>>>> squash and force push.
>>>>
>>>>  I'd just leave the multiple commits.  We can squash them later.
>>>>
>>>> On Tue, Apr 7, 2015 at 1:59 PM, John O'Hara <johara at redhat.com> wrote:
>>>>
>>>>>  Steve,
>>>>>
>>>>> I have made changes based on the github comments and your comments
>>>>> below (https://github.com/johnaoahra80/hibernate-orm/commits/HHH-9701
>>>>> ).
>>>>>
>>>>> Do you want me to squash the commits down to one? Not sure how this
>>>>> would effect the comments you have already made on GH
>>>>>
>>>>> Thanks
>>>>>
>>>>> John
>>>>>
>>>>>
>>>>> On 06/04/15 04:51, Steve Ebersole wrote:
>>>>>
>>>>>
>>>>>
>>>>> On Thu, Apr 2, 2015 at 4:45 AM, John O'Hara <johara at redhat.com> wrote:
>>>>>
>>>>>>  Steve,
>>>>>>
>>>>>> I have pushed a proposal for HHH-9701 to:
>>>>>> https://github.com/johnaoahra80/hibernate-orm/tree/HHH-9701
>>>>>>
>>>>>> There are a couple of areas that I would appreciate feedback;
>>>>>>
>>>>>> 1) Serialization/Deserialization - EntityEntries implementations can
>>>>>> be serialized and each implementation provide their own serialization
>>>>>> method. I have modified the serialization of EntityEntry in
>>>>>> EntityEntryContext to write the Implementation class to the OutputStream so
>>>>>> the correct class can be used to deserialize the object stream.  Is the
>>>>>> exception handling sufficient here, or do we need more robust handling of
>>>>>> deserialization exceptions? : see
>>>>>> https://github.com/johnaoahra80/hibernate-orm/commit/ec9b1fa3b97131ff1e65a1cc30ff6e4f2d2c8a28#diff-b55e1e51b30abe8d3c280bb22aeb6a44R380
>>>>>>
>>>>>
>>>>>  I added some comments to that section.  Also, overall I would
>>>>> extract the deserialization bit into a separate method
>>>>> (deserializeEntityEntry).
>>>>>
>>>>>
>>>>>
>>>>>  2) In our (perf team) use case, we want to be able to share the
>>>>>> ImmutableEntityEntry between sessions when they are referenced cached in
>>>>>> the 2lc. I have modified EntityEntryContext to not null
>>>>>> managedEntity.$$_hibernate_setEntityEntry if the EntityEntry is an instance
>>>>>> of ImmutableEntityEntry.  Do we need to add an extra checks here, to ensure
>>>>>> that the entity is Reference Cached? I am not sure how we would test that
>>>>>> case? : see
>>>>>> https://github.com/johnaoahra80/hibernate-orm/commit/ec9b1fa3b97131ff1e65a1cc30ff6e4f2d2c8a28#diff-b55e1e51b30abe8d3c280bb22aeb6a44L281
>>>>>>
>>>>>
>>>>>  It really depends on what y'all consider the trigger for using
>>>>> ImmutableEntityEntry.  When would the EntityPersister use the
>>>>> EntityEntryFactory producing ImmutableEntityEntry instances?
>>>>> a) when the entity is marked immutable?
>>>>> b) when the entity is marked immutable *and* we need to cache it by
>>>>> reference?
>>>>> c) some other condition?
>>>>>
>>>>>  I agree that we should only not clear that reference when the entity
>>>>> is enabled for cache-by-reference.  How that plays into this depends on the
>>>>> answer to the above question.
>>>>>
>>>>>  If (a), then I think that yes it makes sense to add a check to only
>>>>> clear the ManagedEntity's EntityEntry reference if using cache-by-reference.
>>>>>
>>>>>  If (b), then the EntiytPersister is only using the
>>>>> EntityEntryFactory producing ImmutableEntityEntry instances when both are
>>>>> true.  So the fact that an entry is an instance of ImmutableEntityEntry
>>>>> indicates that we need to not clear it from the ManagedEntity.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> 3) Lock Mode: Steve you mentioned about not doing locking for
>>>>>> Immutable entities. Where is the locking implemented? Would it be
>>>>>> sufficient to simply set the LockMode on the ImmutableEntityEntry to
>>>>>> NONE/READ_ONLY when setLockMode is called?
>>>>>>
>>>>>
>>>>>
>>>>>  Locking is implemented in many places.
>>>>>
>>>>>  What I had in mind, in terms of implementation for EntityEntry, is
>>>>> somewhat influenced by the choice between ignore versus exception in cases
>>>>> where something is not supported.  Basically I had thought to throw an
>>>>> exception in ImmutableEntityEntry#setLockMode or to simply ignore the call
>>>>> altogether.  This is not a great solution.
>>>>>
>>>>>  It is hard for me to justify ignoring the lock request in all
>>>>> cases.  What does everyone else think?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>   --
>>>>> John O'Harajohara 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).
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> John O'Harajohara 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).
>>>>
>>>>
>>>>
>>>
>>
>>
>> --
>> John O'Harajohara 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).
>>
>>
>>
>
>
> --
> John O'Harajohara 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).
>
>
>