You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@santuario.apache.org by "Deltoro, Salvador" <sa...@hp.com> on 2004/06/03 13:46:57 UTC
[Patch] Patch proposed for solving memory retention problem in XMLSecurity 1.1.0 java library
Hello.
We have been doing exhaustive performance tests of an application that
uses the XMLSecurity 1.1.0 java library and we have found out a memory
retention problem in this library.
The problem is produced by the class
org.apache.xml.security.utils.IdResolver.
The reason lies inside the method "public static void
registerElementById(Element element, String idValue)", specifically in
this line:
>>
elementMap.put(idValue, element);
>>
This instruction adds elements to a WeakHashMap using "strong"
references, not "weak" references (please see
http://java.sun.com/j2se/1.4.2/docs/api/java/util/WeakHashMap.html for
details). (These elements contain xerces DeferredDocumentImpl objects).
The JVM's garbage collector will automatically remove from the memory
heap all the objects referenced "weakly" that are no longer used, but
not the objects referenced "strongly".
Therefore, this instruction provokes that the gc cannot remove all
objects from memory, so each invocation to the IdResolver class leaves
new objects in the heap that cannot removed after use. The result is
that the heap consumption grows constantly until the maximum configured
for the JVM, making the performance degrade more and more.
Our environment is based on HP-UX SDK 1.4.2_03.
We have developed locally a patch in order to solve this issue that has
worked perfectly well under performance testing. I have substituted the
former instruction by this one:
>>
elementMap.put(idValue, new java.lang.ref.WeakReference(element));
>>
In this way, the IdResolver class puts the element as a explicit "weak"
reference, so the garbage collector can remove it when no longer used
and
We think that this XMLSecurity 1.1.0 performance problem can pose a big
hurdle for using it broadly in real production environments, so it is
worth that the persons responsible for the code repository assess the
suitability of including this patch into XMLSecurity main development
line in order to be included in next releases.
We look forward to your feedback regarding this patch proposal. Please
let me know if you need more information on this.
Regards,
Salvador Deltoro.
Re: [Patch] Patch proposed for solving memory retention problem in XMLSecurity 1.1.0 java library
Posted by ki...@engr.orst.edu.
Quoting Raul Benito <ra...@r-bg.com>:
> >... A more
> >degenerate case is if the value object strongly references the key, but I
> doubt
> >that is the case here.
>
> This is exactly what is happening in one call path, see this code
> ...
> If you see when the setId is called it is called the Key is referenced
> by the value in a hard way.
> We can modify the put or we can modify this call but it seams a memory
> leak that needs to be fixed.
Yes, can't argue with that (I'm not familiar with the code, I was just speaking
from a theoretical standpoint). Fixing the put or call is probably the
'better' solution, though.
> It is not a real problem because it is only a cached optimization, if it
> is not found in the table, the document is search again with XPath
> expressions.
Okay - just *looked* dangerous, but apparently not. Still, if changing the
reference type of the value objects is the preferred solution, it would
probably be better to use soft references instead of weak references (as with a
commons-collections reference map). Then they would only be reclaimed when the
memory is actually needed instead of every time the gc runs.
Re: [Patch] Patch proposed for solving memory retention problem in
XMLSecurity 1.1.0 java library
Posted by Raul Benito <ra...@r-bg.com>.
kinneer@engr.orst.edu wrote:
>IMHO, the current usage of WeakHashMap in the library is correct. The effect of
>the current usage is that when there are no more active references to the key,
>the mapping can be removed, which means the map no longer has a strong
>reference to the value object. Therefore if the garbage collector determines
>that the value object can be reclaimed, it won't be blocked from doing so by
>the presence of the mapping (as in a normal HashMap); this holds true
>regardless of whether the value objects are strongly referenced in the map.
>
>You may still have problems with memory retention, however, if strong references
>to the keys are not being released when they should (or in a timely manner), or
>if other strong references to the value object are not being released (in which
>case it is not the fault of the WeakHashMap implementation or its current
>usage). Either one of these may be issues to check for elsewhere. A more
>degenerate case is if the value object strongly references the key, but I doubt
>that is the case here.
>
>
This is exactly what is happening in one call path, see this code
XMLSignature(but the same code is Manifest,Reference, and several others)
/**
* Sets the <code>Id</code> attribute
*
* @param Id Id value to be used by the id attribute on the Signature
Element
*/
public void setId(String Id) {
if ((this._state == MODE_SIGN) && (Id != null)) {
this._constructionElement.setAttributeNS(null,
Constants._ATT_ID, Id);
IdResolver.registerElementById(this._constructionElement, Id);
}
}
If you see when the setId is called it is called the Key is referenced
by the value in a hard way.
We can modify the put or we can modify this call but it seams a memory
leak that needs to be fixed.
>In any case, the proposed patch appears to me to be dangerous. It effectively
>converts the map to a soft-memory cache, where values of the mappings may be
>cleared at any time once they are no longer referenced anywhere else. This
>means that attempts to retrieve mappings may unexpectedly return null
>references (the key still exists but the value has been gc'ed), which probably
>is not expected behavior for this map and could cause failures in parts of the
>code that use it.
>
>
>
It is not a real problem because it is only a cached optimization, if it
is not found in the table, the document is search again with XPath
expressions.
Anyhow when dom3 api will be released and used this function can be
removed and used the setAttributeId API in DOM, instead of the cache table.
Re: [Patch] Patch proposed for solving memory retention problem in XMLSecurity 1.1.0 java library
Posted by ki...@engr.orst.edu.
IMHO, the current usage of WeakHashMap in the library is correct. The effect of
the current usage is that when there are no more active references to the key,
the mapping can be removed, which means the map no longer has a strong
reference to the value object. Therefore if the garbage collector determines
that the value object can be reclaimed, it won't be blocked from doing so by
the presence of the mapping (as in a normal HashMap); this holds true
regardless of whether the value objects are strongly referenced in the map.
You may still have problems with memory retention, however, if strong references
to the keys are not being released when they should (or in a timely manner), or
if other strong references to the value object are not being released (in which
case it is not the fault of the WeakHashMap implementation or its current
usage). Either one of these may be issues to check for elsewhere. A more
degenerate case is if the value object strongly references the key, but I doubt
that is the case here.
In any case, the proposed patch appears to me to be dangerous. It effectively
converts the map to a soft-memory cache, where values of the mappings may be
cleared at any time once they are no longer referenced anywhere else. This
means that attempts to retrieve mappings may unexpectedly return null
references (the key still exists but the value has been gc'ed), which probably
is not expected behavior for this map and could cause failures in parts of the
code that use it.
> We have been doing exhaustive performance tests of an application that
> uses the XMLSecurity 1.1.0 java library and we have found out a memory
> retention problem in this library.
>
> The problem is produced by the class
> org.apache.xml.security.utils.IdResolver.
>
> The reason lies inside the method "public static void
> registerElementById(Element element, String idValue)", specifically in
> this line:
>
> >>
> elementMap.put(idValue, element);
> >>
>
> This instruction adds elements to a WeakHashMap using "strong"
> references, not "weak" references (please see
> http://java.sun.com/j2se/1.4.2/docs/api/java/util/WeakHashMap.html for
> details). (These elements contain xerces DeferredDocumentImpl objects).
> The JVM's garbage collector will automatically remove from the memory
> heap all the objects referenced "weakly" that are no longer used, but
> not the objects referenced "strongly".
>
> Therefore, this instruction provokes that the gc cannot remove all
> objects from memory, so each invocation to the IdResolver class leaves
> new objects in the heap that cannot removed after use. The result is
> that the heap consumption grows constantly until the maximum configured
> for the JVM, making the performance degrade more and more.
>
> Our environment is based on HP-UX SDK 1.4.2_03.
>
> We have developed locally a patch in order to solve this issue that has
> worked perfectly well under performance testing. I have substituted the
> former instruction by this one:
>
> >>
> elementMap.put(idValue, new java.lang.ref.WeakReference(element));
> >>
>
> In this way, the IdResolver class puts the element as a explicit "weak"
> reference, so the garbage collector can remove it when no longer used
> and
>
> We think that this XMLSecurity 1.1.0 performance problem can pose a big
> hurdle for using it broadly in real production environments, so it is
> worth that the persons responsible for the code repository assess the
> suitability of including this patch into XMLSecurity main development
> line in order to be included in next releases.
>
> We look forward to your feedback regarding this patch proposal. Please
> let me know if you need more information on this.
>
> Regards,
> Salvador Deltoro.
>