I haven't had a chance to run the patch though big performance tests
yet, as I'm still trying to find the other memory leaks. I can see if
there are cycles on the alacrity systems tonight and run it there.
So I don't know that it is a bottleneck, but if the list averages 1-2
EMs per client, then I can certainly see that happening. In the patch
Mitesh originally gave me to test, the list was only traversed every 100
additions; this newer code will traverse everytime. Is this the
now-proposed implementation?
-Scott
On Wed, 2006-08-30 at 15:09, Marina Vatkina wrote:
> Scott,
>
> We don't need the list at all if EM checks EMF if it's closed, then
> close itself on all public APIs. But this is a bigger change and we
> were wondering if you can see the list as the bottleneck (i.e. is it
> worth spending time on changing the code)?
>
> thanks,
> -marina
>
> Scott Oaks wrote:
> > If there are 1000s of clients accessing the application, does the list
> > have all the client EMs? Or is there a different factory for each
> > client? I'm a little concerned that cycling through the list could
> > become expensive if there are that many simultaneous transactions -- and
> > then the list becomes its own bottleneck.
> >
> > I'm actually not sure about this entire thing: why can't the em.close()
> > method remove itself from the list? Then the list wouldn't need to have
> > WeakReferences at all, and when (if) the factory is closed, it can deal
> > with the existing EMs.
> >
> > -Scott
> >
> > On Wed, 2006-08-30 at 14:28, Mitesh Meswani wrote:
> >
> >>Gordon Yorke wrote:
> >>
> >>>Hello Mitesh,
> >>> This should be suffecient. We could move the code to createEntityManager instead of close(). That would ensure that the references are always cleaned up.
> >>>
> >>
> >>Good suggestion. It can help remove the synchronized blocks guarding
> >>entityManagers. However, the list "entityManagers" would keep on growing
> >>till a gc kicks in and we would be iterating the growing list every time
> >>we create an entitymanager. With current approach for a well behaved
> >>application, the list would be trimmed for each close. So, the choice is
> >>between eliminating synchronized blocks or reducing iteration size.
> >>
> >>Scott,
> >>Any suggestions on which approach might be better? Attaching the patch
> >>again for your reference
> >>
> >>Thanks,
> >>Mitesh
> >>
> >>
> >>
> >>>--Gordon
> >>>
> >>>-----Original Message-----
> >>>From: Mitesh.Meswani_at_Sun.COM [mailto:Mitesh.Meswani_at_Sun.COM]On Behalf Of
> >>>Mitesh Meswani
> >>>Sent: Tuesday, August 29, 2006 8:58 PM
> >>>To: Gordon Yorke; Tom Ware
> >>>Cc: persistence_at_glassfish.dev.java.net
> >>>Subject: Code review for issue 1003
> >>>
> >>>
> >>>Hi Gordon,
> >>>
> >>>Please find attached a fix for issue 1003
> >>><https://glassfish.dev.java.net/issues/show_bug.cgi?id=1033>.
> >>>Following is a summary:
> >>>Changed the EntityManagerFactoryImpl.#entityManagers to be a linked list
> >>>Added a new method emClosed() to EntityManagerFactoryImpl. This method
> >>>is called from EntityManagerImpl.close(). The method iterates over the
> >>>list and cleans up as required.
> >>>
> >>>Tests run:
> >>>A local test exercising the code.
> >>>Scott Oaks is running Specj with this code.
> >>>
> >>>This bug is one of the show stoppers for the UR1 release which has a HCF
> >>>tomorrow (Wed). :(
> >>>
> >>>Please note that the fix has a limitation of not cleaning up the list in
> >>>EntityManagerFactoryImpl if none of the ems of an application call
> >>>close(). The implementation to solve it is a bit more involved. At this
> >>>point I am wondering whether we need to maintain the list of
> >>>entitymanagers at all in. I can implement the involved fix if
> >>>maintaining the list is really required.
> >>>
> >>>Thanks,
> >>>Mitesh
> >>>
> >>>
> >>>