And change the name of DefaultEntityEntryFactory :) On Fri, Apr 17, 2015 at 2:04 PM, John O'Hara <joh...@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 <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). >> >> >> > > > -- > 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
