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 <joh...@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 <st...@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 <joh...@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 <joh...@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 <joh...@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'harajoh...@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'harajoh...@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'harajoh...@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
