Perhaps I'm being a bit dim, but ...
Why store the Property as a key and a value both? Just store "" as the
value. You can still get a list of all the cached Property objects by
calling keySet(). The entries in the Set returned may be WeakReference
objects, but that's easily dealt with.
Just my 2 cents.
-- Mark C. Allman,
PMP
-- Allman
Professional
Consulting, Inc.
-- 617-947-4263
--
www.allmanpc.com
On Fri, 2007-07-20 at 14:53 +0100, Adrian Cumiskey wrote:
> Taking a look at the code and reading the spec :-
>
> public class PropertyCache {
>
> private Map propCache = Collections.synchronizedMap(new WeakHashMap());
>
>
> /**
> * Checks if the given property is present in the cache - if so,
> returns
> * a reference to the cached value. Otherwise the given object is
> added
> * to the cache and returned.
> * @param obj
> * @return the cached instance
> */
> public Property fetch(Property prop) {
>
> Property cacheEntry = (Property) propCache.get(prop);
> if (cacheEntry != null) {
> return cacheEntry;
> } else {
> propCache.put(prop, prop);
> return prop;
> }
> }
> }
>
> "Implementation note: The value objects in a WeakHashMap are held by
> ordinary strong references. Thus care should be taken to ensure that
> value objects do not strongly refer to their own keys, either directly
> or indirectly, since that will prevent the keys from being discarded.
> Note that a value object may refer indirectly to its key via the
> WeakHashMap itself; that is, a value object may strongly refer to some
> other key object whose associated value object, in turn, strongly refers
> to the key of the first value object. One way to deal with this is to
> wrap values themselves within WeakReferences before inserting, as in:
> m.put(key, new WeakReference(value)), and then unwrapping upon each get."
>
> I have to agree with Jeremias, the value does need to be wrapped in a
> WeakReference object when it is put() in the PropertyCache as the value
> reference will be held as a strong reference as it directly holds the
> same reference value as its key.
>
> This patch should single line patch should fix it.
>
> -- snip --
> Index: src/java/org/apache/fop/fo/properties/PropertyCache.java
> ===================================================================
> --- src/java/org/apache/fop/fo/properties/PropertyCache.java
> (revision 555659)
> +++ src/java/org/apache/fop/fo/properties/PropertyCache.java (working
> copy)
> @@ -19,6 +19,7 @@
>
> package org.apache.fop.fo.properties;
>
> +import java.lang.ref.WeakReference;
> import java.util.Collections;
> import java.util.Map;
> import java.util.WeakHashMap;
> @@ -48,7 +49,7 @@
> if (cacheEntry != null) {
> return cacheEntry;
> } else {
> - propCache.put(prop, prop);
> + propCache.put(prop, new WeakReference(prop));
> return prop;
> }
> }
> -- snip --
>
>
> Adrian.
>
> Chris Bowditch wrote:
> > Jeremias Maerki wrote:
> >
> >> On 20.07.2007 12:51:33 Chris Bowditch wrote:
> >>
> >>> Jeremias Maerki wrote:
> >>>
> >>>
> >>>> On 20.07.2007 11:52:15 Andreas L Delmelle wrote:
> >>>
> >>> <snip/>
> >>>
> >>>>>> In addition to that there was a bug in FixedLength.equals() that made
> >>>>>> the caching effect-less:
> >>>>>> http://svn.apache.org/viewvc?view=rev&rev=557934
> >>>>>
> >>>>> That was the most likely cause. The equals() method returning
> >>>>> false because of this, would keep on creating separate instances.
> >>>>> How they would be leaked into a subsequent run is not quite clear
> >>>>> to me, though...
> >>>>
> >>>>
> >>>> Because of the bug in PropertyCache. The two bugs just add to each
> >>>> other.
> >>>
> >>> Thanks for spotting this bug in FixedLength.equals(). I rebuilt
> >>> fop.jar and started a fresh profiling session. After 5000 renderings
> >>> with only 16Mb the heap is staying around 3Mb. The number of
> >>> FixedLength and WeakHashMap$entry object is staying fixed at around
> >>> 300. Hip Hip Hooray!!!
> >>
> >>
> >> But you're running the same document over and over again, right? In that
> >> case, the WeakHashMap doesn't grow because the cached instances are
> >> always reused. That doesn't address the fact that the instances are
> >> still not releasable. The memory leak is still there. That's why it's so
> >> extremely important to be so damn careful about using static variables.
> >
> > You are right of course. So if we ran lots of different documents with a
> > very high number of variations in FixedLength then the memory leak still
> > exists. It does sound to me from what you've been saying about the
> > WekHashMap that more work in this area is still needed.
> >
> > <snip/>
> >
> > Chris
> >
> >
> >
>
