Scott Oaks wrote:
> 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?
>
I figured that if the application is well behaved (that is closes the
em) the list would not grow beyond open ems (which is open transactions)
for a given emf. Thats why I removed the optimization for list traversal.
After discussions with Gordon, if maintaining of the list is absolutely
required the "final" solution I have in mind would use a ReferenceQueue
to poll for the eligible reference. It might require more testing hence
I am not checking it in today.
Thanks,
Mitesh
> -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
>>>>>
>>>>>
>>>>>
>>>>>