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 --


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:


In addition to that there was a bug in FixedLength.equals() that made
the caching effect-less:

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.



Reply via email to