You are viewing a plain text version of this content. The canonical link for it is here.
Posted to fop-dev@xmlgraphics.apache.org by Chris Bowditch <bo...@hotmail.com> on 2007/07/19 16:37:10 UTC
OutOfMemoryError in current Trunk
Fellow Devs,
I have been doing a little bit of testing with the current Trunk code
and observed that I can generate an OutOfMemoryError simply by
submitting the same 2 page document to FOP to render to Postscript over
and over again. After just 2,900 renderings FOP dies with
OutOfMemoryError. I think it is safe to assume that the memory leak
exists in the current 0.94 branch also. I am keen to avoid releasing FOP
whilst the code contains such a problem.
If anyone is interested I can send you the FO file and test harness code
I use to repeatidly generate PS off list.
Hopefully at some point I will have time to profile FOP and figure out
where the source of the memory leak is, but right now I don't have any
more time, so this e-mail is just FYI.
Chris
Re: OutOfMemoryError in current Trunk
Posted by Adrian Cumiskey <ad...@gmail.com>.
Attached revised fix.
Jeremias Maerki wrote:
> Adrian,
>
> you forgot to account for the case where the GC removes the instance in
> the worst possible moment and WeakReference.get() returns null.
>
> On 20.07.2007 16:33:34 Adrian Cumiskey wrote:
>> 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;
>> @@ -39,16 +40,16 @@
>> * 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
>> + * @param prop a property
>> * @return the cached instance
>> */
>> public Property fetch(Property prop) {
>> -
>> - Property cacheEntry = (Property) propCache.get(prop);
>> - if (cacheEntry != null) {
>> + WeakReference ref = (WeakReference) propCache.get(prop);
>> + if (ref != null) {
>> + Property cacheEntry = (Property)ref.get();
>> return cacheEntry;
>> } else {
>> - propCache.put(prop, prop);
>> + propCache.put(prop, new WeakReference(prop));
>> return prop;
>> }
>> }
>
>
>
> Jeremias Maerki
>
>
Re: OutOfMemoryError in current Trunk
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
Adrian,
you forgot to account for the case where the GC removes the instance in
the worst possible moment and WeakReference.get() returns null.
On 20.07.2007 16:33:34 Adrian Cumiskey wrote:
> 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;
> @@ -39,16 +40,16 @@
> * 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
> + * @param prop a property
> * @return the cached instance
> */
> public Property fetch(Property prop) {
> -
> - Property cacheEntry = (Property) propCache.get(prop);
> - if (cacheEntry != null) {
> + WeakReference ref = (WeakReference) propCache.get(prop);
> + if (ref != null) {
> + Property cacheEntry = (Property)ref.get();
> return cacheEntry;
> } else {
> - propCache.put(prop, prop);
> + propCache.put(prop, new WeakReference(prop));
> return prop;
> }
> }
Jeremias Maerki
Re: OutOfMemoryError in current Trunk
Posted by Adrian Cumiskey <ad...@gmail.com>.
Fixed my patch and attached it.
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
>
>
>
Re: OutOfMemoryError in current Trunk
Posted by Andreas L Delmelle <a_...@pandora.be>.
On Jul 20, 2007, at 17:15, Mark C. Allman wrote:
Hi Mark
> 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.
Interesting idea, but... the point of storing the Property in key and
value, is being able to do for example:
StringProperty cachedInstance = (StringProperty) Map.get(new
StringProperty(...));
I don't immediately see a convenient solution for that using keySet
()... not at first glance, at least. :/
Note:
As indicated earlier in another thread, the cost of construction of
the related Property subtypes is low enough so as not to cause a
significant overhead. If this were not the case, then an entirely
different strategy would have to be used that avoids the cost of
creation altogether.
If the Property is already cached, the 'new' instance in the call
above doesn't have to leave the stack. Its lifetime is limited to the
execution of get(). We would instead get a reference to a
cachedInstance that was already moved to the heap earlier on.
Clever in concept, but still needs some work, I guess.
Cheers
Andreas
Re: OutOfMemoryError in current Trunk
Posted by "Mark C. Allman" <mc...@allmanpc.com>.
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
> >
> >
> >
>
Re: OutOfMemoryError in current Trunk
Posted by Adrian Cumiskey <ad...@gmail.com>.
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
>
>
>
Re: OutOfMemoryError in current Trunk
Posted by Adrian Cumiskey <ad...@gmail.com>.
Whoops, please ignore the patch I just sent, I made a mistake.. :-[
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
>
>
>
Re: OutOfMemoryError in current Trunk
Posted by Chris Bowditch <bo...@hotmail.com>.
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
Re: OutOfMemoryError in current Trunk
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
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.
> Obviously we will continue to run a few more tests before assuming all
> is well, but it certainly appears to have fixed the problem! Funny that
> Andreas never ran into the issue though :-/
>
> Chris
>
Jeremias Maerki
Re: OutOfMemoryError in current Trunk
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
Sorry, but I don't agree. Anyone else?
On 20.07.2007 14:01:52 Andreas L Delmelle wrote:
> On Jul 20, 2007, at 11:58, Jeremias Maerki wrote:
>
> >
> > Read again: "Implementation note: The value objects in a
> > WeakHashMap are
> > held by ordinary strong references." That means that all the object in
> > there can never be released since there never be only a weak reference
> > to the object there.
>
> Right, but the value objects are not the issue. As long as the key
> can be released, and the value does not refer directly or indirectly
> back to the key, there is no problem. What the Javadoc warns about is
> the situation where value /refers back/ to the key in some way or
> other: most simple case would be that the value object is composite
> and has an instance member that is itself the key object. In that
> case, the entry will never be released, unless the value itself is
> wrapped in a WeakReference.
>
> FWIW: I had the same initial reservations, but re-reading the
> Javadoc, it occurred to me that this was actually no problem at all.
>
>
> Cheers
>
> Andreas
Jeremias Maerki
Re: OutOfMemoryError in current Trunk
Posted by Andreas L Delmelle <a_...@pandora.be>.
On Jul 20, 2007, at 16:51, Jeremias Maerki wrote:
> Want to find a memory leak? SAP's memory analyzer is great again to
> find
> out all paths to GC roots:
>
> <snip />
> As a simplification here, let's assume "cache" (PropertyCache in
> static
> variable) is our GC root. Hard references are to "propCache", then
> "m" and
> "table". From there it goes to the WeakHashMap$Entry in the array
> with a
> hard reference. From the Entry, you have a hard reference (!) to the
> FixedLength instance through "value" and a weak reference through
> "referent" (which you can find in java.lang.ref.Reference). QED.
Yep, I see the point. QED indeed.
> If this were my current focus, I'd remove PropertyCache in favor of
> specialized and optimized FlyWeightFactories and I'd attach those
> to the
> rendering run so I can get rid of the need for thread synchronization
> and weak references. I'd do some isolated timing experiments comparing
> PropertyCache and the FlyWeightFactories. But since this has to wait
> until later this year, I guess I'm happy enough if you guys trying out
> Adrian's suggestion in order to get a quick solution.
Adrian's suggestion should be enough FTM, to guarantee that even in
exotic cases with a huge amount of distinct property values in long
series of subsequent and concurrent runs, there are no leaks. I just
tried it out, and now, after finishing a run only the mentioned
initial values referenced by the Makers, are retained in the Map. The
specified values particular to the run are now correctly released
after the run has finished.
Cheers
Andreas
Re: OutOfMemoryError in current Trunk
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
Want to find a memory leak? SAP's memory analyzer is great again to find
out all paths to GC roots:
Class Name |Shallow Heap |Retained Heap
---------------------------------------------------------------------------------------------------------------------------------------------
org.apache.fop.fo.properties.FixedLength @ 0x6ad2d98 | 24| 24
'- value, referent java.util.WeakHashMap$Entry @ 0x6aeee30 | 40| 120
'- [90] java.util.WeakHashMap$Entry[128] @ 0x6abc818 | 528| 3'592
'- table java.util.WeakHashMap @ 0x6a0b2f0 | 48| 3'672
'- m java.util.Collections$SynchronizedMap @ 0x6a0b2d0 | 32| 3'704
'- propCache org.apache.fop.fo.properties.PropertyCache @ 0x6a0b2c0 | 16| 3'720
'- cache class org.apache.fop.fo.properties.FixedLength @ 0x31b26a0 | 8| 3'728
|- <class> org.apache.fop.fo.properties.FixedLength @ 0x6ab3ff0 | 24| 24
| '- textIndent, lastLineEndIndent org.apache.fop.fo.flow.Block @ 0xa61ead8 | 184| 496
| |- currentFObj org.apache.fop.fo.FOTreeBuilder$MainFOHandler @ 0x6ab53a0 [Java Local]| 24| 24
| | '- java.lang.Thread @ 0x6a2aa10 main [Thread] | 104| 704
| '- parent org.apache.fop.fo.flow.Block @ 0xa61fb48 [Java Local] | 184| 184
|- <class> org.apache.fop.fo.properties.FixedLength @ 0xa61f6c8 | 24| 24
|- [555] java.lang.Object[1280] @ 0x6ab9100 | 5'136| 202'392
'- <class> org.apache.fop.fo.properties.FixedLength @ 0x6ab4108 | 24| 24
---------------------------------------------------------------------------------------------------------------------------------------------
As a simplification here, let's assume "cache" (PropertyCache in static
variable) is our GC root. Hard references are to "propCache", then "m" and
"table". From there it goes to the WeakHashMap$Entry in the array with a
hard reference. From the Entry, you have a hard reference (!) to the
FixedLength instance through "value" and a weak reference through
"referent" (which you can find in java.lang.ref.Reference). QED.
If this were my current focus, I'd remove PropertyCache in favor of
specialized and optimized FlyWeightFactories and I'd attach those to the
rendering run so I can get rid of the need for thread synchronization
and weak references. I'd do some isolated timing experiments comparing
PropertyCache and the FlyWeightFactories. But since this has to wait
until later this year, I guess I'm happy enough if you guys trying out
Adrian's suggestion in order to get a quick solution.
On 20.07.2007 16:12:45 Andreas L Delmelle wrote:
> On Jul 20, 2007, at 14:15, Jeremias Maerki wrote:
>
> > Another argument: Why did Chris have an OutOfMemoryError in the first
> > place that the fix for FixedLengthProperty solved?
> > The OutOfMemoryError doesn't happen anymore because not so many
> > objects
> > are created to actually be filling the memory.
>
> That was precisely the point, since:
> a) specified properties very frequently have recurring values in the
> same document as well as over different documents.
> b) default/initial values are generally the same for a given property
> in all documents
> c) computed values for inherited properties are frequently identical
> on parent and child
>
> Even if you render a million documents, there would always be one and
> only one FixedLength instance corresponding to an absolute computed
> value of 10pt, no matter if it is used for font-size, line-height,
> padding...
>
> > If the objects in the cache had been releasable, they would have
> > been released
> > by the GC and Chris wouldn't have run into the OutOfMemoryError.
>
> Depends on how you look at it:
> I don't believe the reference to the value object that is held by the
> Map.Entry object itself is of issue here.
> However, since that is a strong reference (what the API doc means to
> say), then if the value would in turn /hold/ (not /be/) a reference
> to the key object, the references would become circular and the fact
> that the key is wrapped in a WeakReference does not do anything useful.
> If key and value are the same objects, there is no circular
> reference, hence no particular problem. The only strong reference to
> the key is then ultimately held by the *Map.Entry*, not the value
> object (which happens to be the key).
>
> Could be I'm incorrect in my interpretation here. If I am, then the
> solution could still be pretty simple (as I see Adrian has already
> suggested):
> replace put(key, value) with put(key, new WeakReference(value));
>
> That would make sure that, even if the WeakHashMap implementation
> cannot resolve situations where one of its entries contains the only
> strong reference to the key --which I doubt to be the meaning of the
> note, but could be wrong--, the entries are always cleared when no
> objects outside the Map refer to the key.
>
> Another note:
> As to /when precisely/ an entry in a WeakHashMap /does/ get cleared
> by GC, that is guaranteed nowhere. The only guarantee is that "an
> entry in a WeakHashMap will automatically be removed when its key is
> no longer in ordinary use". So, it is not because there are no
> references to the key that the entry will be /immediately/ cleared,
> only that it becomes eligible for collection in the next GC cycle.
>
> This could make it JVM-dependent, and may perhaps explain why I could
> not reproduce the bug.... Some JVMs are better at collecting obsolete
> objects held in static Maps than others. If this is a deciding
> factor, I'd only expect this to be a problem in older versions (less
> advanced GC algorithms).
>
> I'm also thinking, maybe the fact that instances corresponding to
> default/initial values are, in turn, cached by the static
> PropertyMakers in FOPropertyMapping causes carry-over of instances
> into other runs.
>
> (check:
> PropertyMaker.make(PropertyList) {
> if (default != null) {
> return default;
> } else {
> ...
> default = p;
> }
> )
>
> That would make instances corresponding to an initial value indeed
> non-collectable once FOP has processed one document.
> Unless when there are disastrous bugs like the one in FixedLength,
> nothing alarming, since those instances will be used many times over
> after that.
>
>
> FWIW: I agree very much that the caches should probably be made
> global in another way, either tie them to the FopFactory somehow, or
> to a specific rendering run, as you suggest. The first would have the
> benefit of being able to re-use the same NumberProperty instance in
> different subsequent renderings.
>
> I experimented a bit with a Map of (Class->Map) mappings, where the
> key in the outer map is the Property-subclass, and the inner Map
> contains the canonical instances for that type of Property. As I
> remember this was quite slow (presumably due to the increased number
> of lookups), so I decided not to employ that particular strategy.
>
> That said, I'm still very open to suggestions.
>
>
> Cheers
>
> Andreas
Jeremias Maerki
Re: OutOfMemoryError in current Trunk
Posted by Andreas L Delmelle <a_...@pandora.be>.
On Jul 20, 2007, at 14:15, Jeremias Maerki wrote:
> Another argument: Why did Chris have an OutOfMemoryError in the first
> place that the fix for FixedLengthProperty solved?
> The OutOfMemoryError doesn't happen anymore because not so many
> objects
> are created to actually be filling the memory.
That was precisely the point, since:
a) specified properties very frequently have recurring values in the
same document as well as over different documents.
b) default/initial values are generally the same for a given property
in all documents
c) computed values for inherited properties are frequently identical
on parent and child
Even if you render a million documents, there would always be one and
only one FixedLength instance corresponding to an absolute computed
value of 10pt, no matter if it is used for font-size, line-height,
padding...
> If the objects in the cache had been releasable, they would have
> been released
> by the GC and Chris wouldn't have run into the OutOfMemoryError.
Depends on how you look at it:
I don't believe the reference to the value object that is held by the
Map.Entry object itself is of issue here.
However, since that is a strong reference (what the API doc means to
say), then if the value would in turn /hold/ (not /be/) a reference
to the key object, the references would become circular and the fact
that the key is wrapped in a WeakReference does not do anything useful.
If key and value are the same objects, there is no circular
reference, hence no particular problem. The only strong reference to
the key is then ultimately held by the *Map.Entry*, not the value
object (which happens to be the key).
Could be I'm incorrect in my interpretation here. If I am, then the
solution could still be pretty simple (as I see Adrian has already
suggested):
replace put(key, value) with put(key, new WeakReference(value));
That would make sure that, even if the WeakHashMap implementation
cannot resolve situations where one of its entries contains the only
strong reference to the key --which I doubt to be the meaning of the
note, but could be wrong--, the entries are always cleared when no
objects outside the Map refer to the key.
Another note:
As to /when precisely/ an entry in a WeakHashMap /does/ get cleared
by GC, that is guaranteed nowhere. The only guarantee is that "an
entry in a WeakHashMap will automatically be removed when its key is
no longer in ordinary use". So, it is not because there are no
references to the key that the entry will be /immediately/ cleared,
only that it becomes eligible for collection in the next GC cycle.
This could make it JVM-dependent, and may perhaps explain why I could
not reproduce the bug.... Some JVMs are better at collecting obsolete
objects held in static Maps than others. If this is a deciding
factor, I'd only expect this to be a problem in older versions (less
advanced GC algorithms).
I'm also thinking, maybe the fact that instances corresponding to
default/initial values are, in turn, cached by the static
PropertyMakers in FOPropertyMapping causes carry-over of instances
into other runs.
(check:
PropertyMaker.make(PropertyList) {
if (default != null) {
return default;
} else {
...
default = p;
}
)
That would make instances corresponding to an initial value indeed
non-collectable once FOP has processed one document.
Unless when there are disastrous bugs like the one in FixedLength,
nothing alarming, since those instances will be used many times over
after that.
FWIW: I agree very much that the caches should probably be made
global in another way, either tie them to the FopFactory somehow, or
to a specific rendering run, as you suggest. The first would have the
benefit of being able to re-use the same NumberProperty instance in
different subsequent renderings.
I experimented a bit with a Map of (Class->Map) mappings, where the
key in the outer map is the Property-subclass, and the inner Map
contains the canonical instances for that type of Property. As I
remember this was quite slow (presumably due to the increased number
of lookups), so I decided not to employ that particular strategy.
That said, I'm still very open to suggestions.
Cheers
Andreas
Re: OutOfMemoryError in current Trunk
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
Another argument: Why did Chris have an OutOfMemoryError in the first
place that the fix for FixedLengthProperty solved? The OutOfMemoryError
doesn't happen anymore because not so many objects are created to
actually be filling the memory. If the objects in the cache had been
releasable, they would have been released by the GC and Chris wouldn't
have run into the OutOfMemoryError.
On 20.07.2007 14:01:52 Andreas L Delmelle wrote:
> On Jul 20, 2007, at 11:58, Jeremias Maerki wrote:
>
> >
> > Read again: "Implementation note: The value objects in a
> > WeakHashMap are
> > held by ordinary strong references." That means that all the object in
> > there can never be released since there never be only a weak reference
> > to the object there.
>
> Right, but the value objects are not the issue. As long as the key
> can be released, and the value does not refer directly or indirectly
> back to the key, there is no problem. What the Javadoc warns about is
> the situation where value /refers back/ to the key in some way or
> other: most simple case would be that the value object is composite
> and has an instance member that is itself the key object. In that
> case, the entry will never be released, unless the value itself is
> wrapped in a WeakReference.
>
> FWIW: I had the same initial reservations, but re-reading the
> Javadoc, it occurred to me that this was actually no problem at all.
>
>
> Cheers
>
> Andreas
Jeremias Maerki
Re: OutOfMemoryError in current Trunk
Posted by Andreas L Delmelle <a_...@pandora.be>.
On Jul 20, 2007, at 11:58, Jeremias Maerki wrote:
>
> Read again: "Implementation note: The value objects in a
> WeakHashMap are
> held by ordinary strong references." That means that all the object in
> there can never be released since there never be only a weak reference
> to the object there.
Right, but the value objects are not the issue. As long as the key
can be released, and the value does not refer directly or indirectly
back to the key, there is no problem. What the Javadoc warns about is
the situation where value /refers back/ to the key in some way or
other: most simple case would be that the value object is composite
and has an instance member that is itself the key object. In that
case, the entry will never be released, unless the value itself is
wrapped in a WeakReference.
FWIW: I had the same initial reservations, but re-reading the
Javadoc, it occurred to me that this was actually no problem at all.
Cheers
Andreas
Re: OutOfMemoryError in current Trunk
Posted by Chris Bowditch <bo...@hotmail.com>.
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!!!
Obviously we will continue to run a few more tests before assuming all
is well, but it certainly appears to have fixed the problem! Funny that
Andreas never ran into the issue though :-/
Chris
Re: OutOfMemoryError in current Trunk
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
On 20.07.2007 11:52:15 Andreas L Delmelle wrote:
> On Jul 20, 2007, at 11:19, Jeremias Maerki wrote:
>
> >>
> >> The initial profile shows that instances of WeakHashMap$entry and
> >> org.apache.fop.fo.properties.FixedLength continually grow with the
> >> life
> >> of JVM.
> >
> > Interesting coincidence:
> > That's exactly what Andreas already wrote about in the other thread
> > about the property cache: The values of the WeakHashMap must not
> > reference objects (or be the same objects) as used in the keys. See
> > javadoc:
>
> Interesting interpretation, but that's not what the Javadoc says, if
> I interpret correctly:
> While "care should be taken to ensure that value objects do not
> strongly refer to their own keys", there seems to be nothing against
> the values being the same objects as the keys. Identity implies no
> reference.
>
> As long as the value-objects do not contain a member that points to
> the key, we should be safe.
Read again: "Implementation note: The value objects in a WeakHashMap are
held by ordinary strong references." That means that all the object in
there can never be released since there never be only a weak reference
to the object there.
> > 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.
Jeremias Maerki
Re: OutOfMemoryError in current Trunk
Posted by Andreas L Delmelle <a_...@pandora.be>.
On Jul 20, 2007, at 11:19, Jeremias Maerki wrote:
>>
>> The initial profile shows that instances of WeakHashMap$entry and
>> org.apache.fop.fo.properties.FixedLength continually grow with the
>> life
>> of JVM.
>
> Interesting coincidence:
> That's exactly what Andreas already wrote about in the other thread
> about the property cache: The values of the WeakHashMap must not
> reference objects (or be the same objects) as used in the keys. See
> javadoc:
Interesting interpretation, but that's not what the Javadoc says, if
I interpret correctly:
While "care should be taken to ensure that value objects do not
strongly refer to their own keys", there seems to be nothing against
the values being the same objects as the keys. Identity implies no
reference.
As long as the value-objects do not contain a member that points to
the key, we should be safe.
> 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...
Cheers
Andreas
Re: OutOfMemoryError in current Trunk
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
On 20.07.2007 10:45:44 Chris Bowditch wrote:
> Andreas L Delmelle wrote:
>
> > On Jul 19, 2007, at 18:15, Andreas L Delmelle wrote:
> >
> >
> > FWIW, I did a quick while(true){...} test with a modified version of
> > our ExampleFO2PDF. With basic documents, I can't seem to reproduce it.
> > The last cycle, I stopped at 10000 renderings, but I'm pretty sure it
> > would still be running if I hadn't. No noticeably slowdown that would
> > point towards the heap reaching its limits. No SVG tried yet.
>
> The initial profile shows that instances of WeakHashMap$entry and
> org.apache.fop.fo.properties.FixedLength continually grow with the life
> of JVM.
Interesting coincidence:
That's exactly what Andreas already wrote about in the other thread
about the property cache: The values of the WeakHashMap must not
reference objects (or be the same objects) as used in the keys. See
javadoc:
http://java.sun.com/j2se/1.3/docs/api/java/util/WeakHashMap.html
But that's exactly how PropertyCache is implemented right now.
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
> We are now going to run a test without the SVG to see if the OOME occurs
> without it.
>
> Chris
>
Jeremias Maerki
Re: OutOfMemoryError in current Trunk
Posted by Chris Bowditch <bo...@hotmail.com>.
Andreas L Delmelle wrote:
> On Jul 19, 2007, at 18:15, Andreas L Delmelle wrote:
>
>
> FWIW, I did a quick while(true){...} test with a modified version of
> our ExampleFO2PDF. With basic documents, I can't seem to reproduce it.
> The last cycle, I stopped at 10000 renderings, but I'm pretty sure it
> would still be running if I hadn't. No noticeably slowdown that would
> point towards the heap reaching its limits. No SVG tried yet.
The initial profile shows that instances of WeakHashMap$entry and
org.apache.fop.fo.properties.FixedLength continually grow with the life
of JVM.
We are now going to run a test without the SVG to see if the OOME occurs
without it.
Chris
Re: OutOfMemoryError in current Trunk
Posted by Andreas L Delmelle <a_...@pandora.be>.
On Jul 19, 2007, at 18:15, Andreas L Delmelle wrote:
> On Jul 19, 2007, at 18:01, Chris Bowditch wrote:
>
>> Andreas L Delmelle wrote:
>>
>>> If there is anything special about that file, you could always
>>> post it, but I'll try to see if I can reproduce with a simple FO
>>> without special features. If I don't succeed, I might ask you to
>>> post your code as well.
>>
>> The FO file has some SVG embedded in fo:instream-foreign-object.
>
> Important thing to check then, is if the problem persists without
> the SVG. That could already give us a clue of the location.
FWIW, I did a quick while(true){...} test with a modified version of
our ExampleFO2PDF. With basic documents, I can't seem to reproduce
it. The last cycle, I stopped at 10000 renderings, but I'm pretty
sure it would still be running if I hadn't. No noticeably slowdown
that would point towards the heap reaching its limits. No SVG tried yet.
Cheers
Andreas
Re: OutOfMemoryError in current Trunk
Posted by Chris Bowditch <bo...@hotmail.com>.
Chris Bowditch wrote:
> Andreas L Delmelle wrote:
>
>> On Jul 19, 2007, at 18:01, Chris Bowditch wrote:
>>
>>> Andreas L Delmelle wrote:
>>>
>>>> If there is anything special about that file, you could always
>>>> post it, but I'll try to see if I can reproduce with a simple FO
>>>> without special features. If I don't succeed, I might ask you to
>>>> post your code as well.
>>>
>>>
>>>
>>> The FO file has some SVG embedded in fo:instream-foreign-object.
>>
>>
>>
>> Important thing to check then, is if the problem persists without the
>> SVG. That could already give us a clue of the location.
>
>
> Agreed. That is the next test we will run.
FWIW, the FO we have also runs out of memory with the SVG removed :-/
Chris
Re: OutOfMemoryError in current Trunk
Posted by Chris Bowditch <bo...@hotmail.com>.
Andreas L Delmelle wrote:
> On Jul 19, 2007, at 18:01, Chris Bowditch wrote:
>
>> Andreas L Delmelle wrote:
>>
>>> If there is anything special about that file, you could always post
>>> it, but I'll try to see if I can reproduce with a simple FO without
>>> special features. If I don't succeed, I might ask you to post your
>>> code as well.
>>
>>
>> The FO file has some SVG embedded in fo:instream-foreign-object.
>
>
> Important thing to check then, is if the problem persists without the
> SVG. That could already give us a clue of the location.
Agreed. That is the next test we will run.
Thanks,
Chris
Re: OutOfMemoryError in current Trunk
Posted by Andreas L Delmelle <a_...@pandora.be>.
On Jul 19, 2007, at 18:01, Chris Bowditch wrote:
> Andreas L Delmelle wrote:
>
>> If there is anything special about that file, you could always
>> post it, but I'll try to see if I can reproduce with a simple FO
>> without special features. If I don't succeed, I might ask you to
>> post your code as well.
>
> The FO file has some SVG embedded in fo:instream-foreign-object.
Important thing to check then, is if the problem persists without the
SVG. That could already give us a clue of the location.
Cheers
Andreas
Re: OutOfMemoryError in current Trunk
Posted by Chris Bowditch <bo...@hotmail.com>.
Andreas L Delmelle wrote:
> On Jul 19, 2007, at 16:37, Chris Bowditch wrote:
>
> Hi Chris
>
>> I have been doing a little bit of testing with the current Trunk code
>> and observed that I can generate an OutOfMemoryError simply by
>> submitting the same 2 page document to FOP to render to Postscript
>> over and over again. After just 2,900 renderings FOP dies with
>> OutOfMemoryError. I think it is safe to assume that the memory leak
>> exists in the current 0.94 branch also. I am keen to avoid releasing
>> FOP whilst the code contains such a problem.
>>
>> If anyone is interested I can send you the FO file and test harness
>> code I use to repeatidly generate PS off list.
>
>
> If there is anything special about that file, you could always post it,
> but I'll try to see if I can reproduce with a simple FO without special
> features. If I don't succeed, I might ask you to post your code as well.
The FO file has some SVG embedded in fo:instream-foreign-object.
>
> Did you, by any chance, noticed a similar effect with other formats
> (which would indicate the leak is localized in the fotree or layout- code)?
Yes, in one of the tests I was generating Area Tree XML with mimic
Renderer set to the AFP Renderer and observed OOME. So I don't think the
output format is relevant here.
One of my colleagues is going to profile FOP. I will keep you all
informed of any findings.
Thanks,
Chris
Re: OutOfMemoryError in current Trunk
Posted by Andreas L Delmelle <a_...@pandora.be>.
On Jul 19, 2007, at 16:37, Chris Bowditch wrote:
Hi Chris
> I have been doing a little bit of testing with the current Trunk
> code and observed that I can generate an OutOfMemoryError simply by
> submitting the same 2 page document to FOP to render to Postscript
> over and over again. After just 2,900 renderings FOP dies with
> OutOfMemoryError. I think it is safe to assume that the memory leak
> exists in the current 0.94 branch also. I am keen to avoid
> releasing FOP whilst the code contains such a problem.
>
> If anyone is interested I can send you the FO file and test harness
> code I use to repeatidly generate PS off list.
If there is anything special about that file, you could always post
it, but I'll try to see if I can reproduce with a simple FO without
special features. If I don't succeed, I might ask you to post your
code as well.
Did you, by any chance, noticed a similar effect with other formats
(which would indicate the leak is localized in the fotree or layout-
code)?
Cheers
Andreas