[#UP-744] PersonDirectory has a memory leak related to caching IPersons in a WeakHashMap

uPortal

PersonDirectory has a memory leak related to caching IPersons in a WeakHashMap

Created: 13/Nov/04 07:29 AM Updated: 03/Jul/07 04:10 PM

Component/s:

Framework

Affects Version/s:

2.4.1

Fix Version/s:

2.4.2

Original Estimate:	Unknown	Remaining Estimate:	Unknown	Time Spent:	Unknown
Environment:	« Hide Operating System: All Platform: All Show » Operating System: All Platform: All

Bugzilla Id:

1787

Description

« Hide

William G. Thompson, Jr. wrote:

> Folks,
>
> I would appreciate any comment/thoughts/etc on the following:
>
> 1) RU is using 2.3.3+ uPortal and like others we are experiencing a
> chronic memory leak
>
>
> 2) As noted early on the list 2.3.3 PersonDirectory has a memory leak
> resulting from the use of a WeakHashMap to cache IPersons
>
>
> 3) The expected (broken) behavior is that as users login an entry is
> added to the WeakHashMap that is never able to be garbage collect
> since the:
> Value (IPerson)
> has a hard references to the
> Key (person.getAttribute(IPerson.USERNAME)
>
>
> 4) this would mean that for sites that have 30,000+ registered users
> this Map could grow to 30,000+ entries at max and would never go down
> (until the JVM is bounced) (anyone have a estimate on the size of
> IPerson?)
>
> Does this sound right?
>
> 5) I am considering simply removing the WeakHashMap altogether with the
> understanding that the only downside is the
> PersonDirectory.getRestrictedPerson(String uid) will have to query the
> data store each time it is invoked.
>
> Am I missing anything other downside/implication?
>
> Thanks for your thoughts.
>
> Cheers,
> Bill

Bill,
Please try replacing WeakHashMap with
org.jasig.portal.utils.WeakValueMap from uP3 sandbox code. That
implementation is tested and should do what was originally indented for
the PersonDir caching.
-peter.

All

Comments

Work Log

Change History

Sort Order:

[ Permlink | « Hide ]

Andrew Petro [14/Nov/04 10:57 AM]

Stealing this bug per discussion on JASIG-DEV list with intent to:

Bill Thompson: [
Andrew, do you want to take up Bug 1787 (PersonDirectory)? You're probably in
the best position at this point to nail it.
]

My response:
Will do. I'll be able to get to this [today].
I plan on going with the proposal to eliminate PersonDirectory IPerson caching
entirely under the theory that this will eliminate (part of) the memory leak
biting Rutgers and (at least part of) the less-than-fully-dynamic user grouping
under PAGS biting the dynamic-PAGS schools.

We can go from that "known-good" state in figuring out whether the loss of
caching introduces unacceptable PersonDirectory performance or whether it's more
than good enough as a baseline to which caching-inclined uPortal adoptors can
re-introduce caching in keeping with where each adoptor specifically wants to be
on the performance/dynamicness/memory conservation trade-off curve.

[ Permlink | « Hide ]

Andrew Petro [14/Nov/04 11:00 AM]

Bill's response to Peter's suggestion of using WeakValueMap is also relevant
documentation of why this bug is being addressed in the way that it is:

Peter Kharchenko wrote: [
Bill, Please try replacing WeakHashMap with org.jasig.portal.utils.WeakValueMap
from uP3 sandbox code. That implementation is tested and should do what was
originally indented for the PersonDirectory caching. ]

Bill responds: [
I think at this point we would be happy simply not caching IPerson for the
purposes of the getRestrictedPerson() call in PersonDirectory.

a) we don't make heavy use of the cache, it is providing little value to us and
consuming memory, which is a problem

b) LDAP queries and primary key selects tend to be very fast

c) WeakReference behavior is not a substitute for a proper caching strategy

I pretty sure this is how we will go locally, but I am open to going a different
direction on the community tree. Thoughts? Is there a downside to not caching
IPersons that I am missing?

I'd like to get this checked in for a 2.4.2 release in the next couple of days.

Cheers,
Bill ]

[ Permlink | « Hide ]

Andrew Petro [14/Nov/04 11:38 AM]

Committed changes to PersonDirectory and to Authentication to remove
PersonDirectory's caching of IPersons.

I recommend we revisit this issue in 2.HEAD under the auspices of the more
general PersonDirectory refactoring. The bug was the memory leak (and perhaps
the lack of dynamism of attributes.) That bug is resolved here; this bugzilla
is Fixed.

The feature is caching of IPerson objects. That is a feature we can now decide
whether is worth adding again in 2.HEAD. The proposed refactored
PersonDirectory offers the hooks to allow IPersonDirectory implementations to
choose to implement caching.

[ Permlink | « Hide ]

Andrew Petro [14/Nov/04 12:21 PM]

Bill Thompson writes:
FYI. I was able to verify this fix using http://www.yourkit.com/.

[ Permlink | « Hide ]

Eric Dalquist [21/Dec/04 08:48 PM]

Removing the cache causes severe performance problems with using PAGs or Aggregated Layouts

[ Permlink | « Hide ]

Eric Dalquist [21/Dec/04 08:55 PM]

Replaced the WeakHashMap which uses the key for the WeakRefrence, with a WeakValueMap which uses the value for the WeakReference.

Testing shows that soon after a user's session expires, which should be the only object lasting for longer than a request with a reference to an IPerson, the IPerson is removed from the cache.