You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tuscany.apache.org by Raymond Feng <en...@gmail.com> on 2009/10/22 00:36:08 UTC
Re: svn commit: r828086 -
Hi,
This can be problematic:
> + private static WeakHashMap cache = new WeakHashMap<String,
> Object>();
Please note the key is String. Unless the string is interned [1], the key
can be GCed even when the class is active.
Another problem is that code now uses hashCode (int) as the key. The
compiler doesn't complain as the cache is defined as "WeakHashMap cache".
The map should be changed to:
private static WeakHashMap<Integer, Object> cache = new WeakHashMap<Integer,
Object>();
[1] http://java.sun.com/javase/6/docs/api/java/lang/String.html#intern()
Thanks,
Raymond
--------------------------------------------------
From: <sl...@apache.org>
Sent: Wednesday, October 21, 2009 9:27 AM
To: <co...@tuscany.apache.org>
Subject: svn commit: r828086 - in /tuscany/branches/sca-java-1.5.2/modules:
core/src/main/java/org/apache/tuscany/sca/core/invocation/
implementation-java/src/main/java/org/apache/tuscany/sca/implementation/java/impl/
interface/src/main/java/org/apache/tuscany/...
> Author: slaws
> Date: Wed Oct 21 16:27:30 2009
> New Revision: 828086
>
> URL: http://svn.apache.org/viewvc?rev=828086&view=rev
> Log:
> TUSCANY-3312 remove circular references where a class is used as the key
> of a map and the value of the map also references the class. The weakness
> of the map never comes into play as there is always a reference to the key
> (held by the value). This all pins the app classloader and causes a leak
> on each app start/stop cycle
>
> Modified:
>
> tuscany/branches/sca-java-1.5.2/modules/core/src/main/java/org/apache/tuscany/sca/core/invocation/SCAProxy.java
>
> tuscany/branches/sca-java-1.5.2/modules/implementation-java/src/main/java/org/apache/tuscany/sca/implementation/java/impl/JavaElementImpl.java
>
> tuscany/branches/sca-java-1.5.2/modules/interface/src/main/java/org/apache/tuscany/sca/interfacedef/impl/DataTypeImpl.java
>
> Modified:
> tuscany/branches/sca-java-1.5.2/modules/core/src/main/java/org/apache/tuscany/sca/core/invocation/SCAProxy.java
> URL:
> http://svn.apache.org/viewvc/tuscany/branches/sca-java-1.5.2/modules/core/src/main/java/org/apache/tuscany/sca/core/invocation/SCAProxy.java?rev=828086&r1=828085&r2=828086&view=diff
> ==============================================================================
> ---
> tuscany/branches/sca-java-1.5.2/modules/core/src/main/java/org/apache/tuscany/sca/core/invocation/SCAProxy.java
> (original)
> +++
> tuscany/branches/sca-java-1.5.2/modules/core/src/main/java/org/apache/tuscany/sca/core/invocation/SCAProxy.java
> Wed Oct 21 16:27:30 2009
> @@ -33,7 +33,7 @@
> // This is a cache containing the proxy class constructor for each
> business interface.
> // This improves performance compared to calling
> Proxy.newProxyInstance()
> // every time that a proxy is needed.
> - private static WeakHashMap cache = new WeakHashMap<Class, Object>();
> + private static WeakHashMap cache = new WeakHashMap<String,
> Object>();
>
> public static Object newProxyInstance(ClassLoader classloader, Class
> aclass[], InvocationHandler invocationhandler)
> throws IllegalArgumentException
> @@ -44,13 +44,13 @@
> // Lookup cached constructor. aclass[0] is the reference's
> business interface.
> Constructor proxyCTOR;
> synchronized(cache) {
> - proxyCTOR = (Constructor) cache.get(aclass[0]);
> + proxyCTOR = (Constructor)
> cache.get(aclass[0].hashCode());
> }
> if(proxyCTOR == null) {
> Class proxyClass = getProxyClass(classloader, aclass);
> proxyCTOR = proxyClass.getConstructor(constructorParams);
> synchronized(cache){
> - cache.put(aclass[0],proxyCTOR);
> + cache.put(aclass[0].hashCode(),proxyCTOR);
> }
> }
> return proxyCTOR.newInstance(new Object[] {
> invocationhandler });
>
> Modified:
> tuscany/branches/sca-java-1.5.2/modules/implementation-java/src/main/java/org/apache/tuscany/sca/implementation/java/impl/JavaElementImpl.java
> URL:
> http://svn.apache.org/viewvc/tuscany/branches/sca-java-1.5.2/modules/implementation-java/src/main/java/org/apache/tuscany/sca/implementation/java/impl/JavaElementImpl.java?rev=828086&r1=828085&r2=828086&view=diff
> ==============================================================================
> ---
> tuscany/branches/sca-java-1.5.2/modules/implementation-java/src/main/java/org/apache/tuscany/sca/implementation/java/impl/JavaElementImpl.java
> (original)
> +++
> tuscany/branches/sca-java-1.5.2/modules/implementation-java/src/main/java/org/apache/tuscany/sca/implementation/java/impl/JavaElementImpl.java
> Wed Oct 21 16:27:30 2009
> @@ -21,6 +21,7 @@
>
> import java.lang.annotation.Annotation;
> import java.lang.annotation.ElementType;
> +import java.lang.ref.WeakReference;
> import java.lang.reflect.AnnotatedElement;
> import java.lang.reflect.Constructor;
> import java.lang.reflect.Field;
> @@ -36,8 +37,8 @@
> public class JavaElementImpl {
> private AnnotatedElement anchor;
> private ElementType elementType;
> - private Class<?> type;
> - private Type genericType;
> + private WeakReference<Class<?>> type;
> + private WeakReference<Type> genericType;
> private int index = -1;
> private String name;
> private Class<? extends Annotation> classifer;
> @@ -51,24 +52,24 @@
> public JavaElementImpl(Class<?> cls) {
> this.anchor = cls;
> this.elementType = ElementType.TYPE;
> - this.type = cls;
> - this.genericType = cls;
> + this.type = new WeakReference<Class<?>>(cls);
> + this.genericType = new WeakReference<Type>(cls);
> this.name = cls.getName();
> }
>
> public JavaElementImpl(Field field) {
> this.anchor = field;
> this.elementType = ElementType.FIELD;
> - this.type = field.getType();
> - this.genericType = field.getGenericType();
> + this.type = new WeakReference<Class<?>>(field.getType());
> + this.genericType = new
> WeakReference<Type>(field.getGenericType());
> this.name = field.getName();
> }
>
> public JavaElementImpl(Constructor<?> constructor, int index) {
> this.anchor = constructor;
> this.elementType = ElementType.PARAMETER;
> - this.type = constructor.getParameterTypes()[index];
> - this.genericType = constructor.getGenericParameterTypes()[index];
> + this.type = new
> WeakReference<Class<?>>(constructor.getParameterTypes()[index]);
> + this.genericType = new
> WeakReference<Type>(constructor.getGenericParameterTypes()[index]);
> this.index = index;
> this.name = "";
> }
> @@ -76,8 +77,8 @@
> public JavaElementImpl(Method method, int index) {
> this.anchor = method;
> this.elementType = ElementType.PARAMETER;
> - this.type = method.getParameterTypes()[index];
> - this.genericType = method.getGenericParameterTypes()[index];
> + this.type = new
> WeakReference<Class<?>>(method.getParameterTypes()[index]);
> + this.genericType = new
> WeakReference<Type>(method.getGenericParameterTypes()[index]);
> this.index = index;
> this.name = "";
> }
> @@ -92,7 +93,7 @@
> */
> public JavaElementImpl(String name, Class<?> type, Class<? extends
> Annotation> classifer) {
> super();
> - this.type = type;
> + this.type = new WeakReference<Class<?>>(type);
> this.name = name;
> this.classifer = classifer;
> }
> @@ -115,7 +116,7 @@
> * @return the genericType
> */
> public Type getGenericType() {
> - return genericType;
> + return genericType.get();
> }
>
> /**
> @@ -129,7 +130,7 @@
> * @return the type
> */
> public Class<?> getType() {
> - return type;
> + return type.get();
> }
>
> public Annotation[] getAnnotations() {
>
> Modified:
> tuscany/branches/sca-java-1.5.2/modules/interface/src/main/java/org/apache/tuscany/sca/interfacedef/impl/DataTypeImpl.java
> URL:
> http://svn.apache.org/viewvc/tuscany/branches/sca-java-1.5.2/modules/interface/src/main/java/org/apache/tuscany/sca/interfacedef/impl/DataTypeImpl.java?rev=828086&r1=828085&r2=828086&view=diff
> ==============================================================================
> ---
> tuscany/branches/sca-java-1.5.2/modules/interface/src/main/java/org/apache/tuscany/sca/interfacedef/impl/DataTypeImpl.java
> (original)
> +++
> tuscany/branches/sca-java-1.5.2/modules/interface/src/main/java/org/apache/tuscany/sca/interfacedef/impl/DataTypeImpl.java
> Wed Oct 21 16:27:30 2009
> @@ -18,6 +18,7 @@
> */
> package org.apache.tuscany.sca.interfacedef.impl;
>
> +import java.lang.ref.WeakReference;
> import java.lang.reflect.Type;
> import java.util.Map;
> import java.util.concurrent.ConcurrentHashMap;
> @@ -43,8 +44,8 @@
> */
> public class DataTypeImpl<L> implements DataType<L> {
> private String dataBinding;
> - private Class<?> physical;
> - private Type genericType;
> + private WeakReference<Class<?>> physical;
> + private WeakReference<Type> genericType;
> private L logical;
> private Map<Class<?>, Object> metaDataMap;
>
> @@ -77,8 +78,8 @@
> public DataTypeImpl(String dataBinding, Class<?> physical, Type
> genericType, L logical) {
> super();
> this.dataBinding = dataBinding;
> - this.physical = physical;
> - this.genericType = genericType;
> + this.physical = new WeakReference<Class<?>>(physical);
> + this.genericType = new WeakReference<Type>(genericType);
> this.logical = logical;
> }
>
> @@ -88,14 +89,14 @@
> * @return the physical type used by the runtime
> */
> public Class<?> getPhysical() {
> - return physical;
> + return physical.get();
> }
>
> /**
> * @param physical the physical to set
> */
> public void setPhysical(Class<?> physical) {
> - this.physical = physical;
> + this.physical = new WeakReference<Class<?>>(physical);
> }
>
> /**
> @@ -103,7 +104,7 @@
> * @return The java generic type
> */
> public Type getGenericType() {
> - return genericType;
> + return genericType.get();
> }
>
> /**
> @@ -111,7 +112,7 @@
> * @param genericType
> */
> public void setGenericType(Type genericType) {
> - this.genericType = genericType;
> + this.genericType = new WeakReference<Type>(genericType);
> }
>
> /**
> @@ -161,9 +162,9 @@
> final int prime = 31;
> int result = 1;
> result = prime * result + ((dataBinding == null) ? 0 :
> dataBinding.hashCode());
> - result = prime * result + ((genericType == null) ? 0 :
> genericType.hashCode());
> + result = prime * result + ((genericType == null ||
> genericType.get() == null) ? 0 : genericType.get().hashCode());
> result = prime * result + ((logical == null) ? 0 :
> logical.hashCode());
> - result = prime * result + ((physical == null) ? 0 :
> physical.hashCode());
> + result = prime * result + ((physical == null || physical.get() ==
> null) ? 0 : physical.get().hashCode());
> return result;
> }
>
> @@ -181,20 +182,20 @@
> return false;
> } else if (!dataBinding.equals(other.dataBinding))
> return false;
> - if (genericType == null) {
> + if (genericType == null || genericType.get() == null) {
> if (other.genericType != null)
> return false;
> - } else if (!genericType.equals(other.genericType))
> + } else if (!genericType.get().equals(other.genericType.get()))
> return false;
> if (logical == null) {
> if (other.logical != null)
> return false;
> } else if (!logical.equals(other.logical))
> return false;
> - if (physical == null) {
> + if (physical == null || physical.get() == null) {
> if (other.physical != null)
> return false;
> - } else if (!physical.equals(other.physical))
> + } else if (!physical.get().equals(other.physical.get()))
> return false;
> return true;
> }
> @@ -219,8 +220,8 @@
> StringBuilder b = new StringBuilder( 256 );
> b.append( "DataType[" );
> b.append( "dataBinding=" + ((dataBinding==null) ? "null" :
> dataBinding) );
> - b.append( ", genericType=" + ((genericType==null) ? "null" :
> genericType) );
> - b.append( ", physical=" + ((physical==null) ? "null" :
> physical) );
> + b.append( ", genericType=" + ((genericType==null ||
> genericType.get()==null) ? "null" : genericType.get()) );
> + b.append( ", physical=" + ((physical==null ||
> physical.get()==null) ? "null" : physical.get()) );
> b.append( ", logical=" + ((logical==null) ? "null" :
> logical) );
> b.append( ", metaData size=" + ((metaDataMap==null) ? "0" :
> metaDataMap.size()) );
> b.append( "]" );
>
>
Re: svn commit: r828086 -
Posted by Raymond Feng <en...@gmail.com>.
What about using a LinkedHashMap with limited size abd LRU policy?
--------------------------------------------------
From: "Simon Laws" <si...@googlemail.com>
Sent: Thursday, October 22, 2009 9:15 AM
To: <de...@tuscany.apache.org>
Subject: Re: svn commit: r828086 -
> Good spot Raymond.
>
> I'm not so concerned about it being a String rather than an Integer
> although I can of course fix that.
>
> Garbage collecting the hash map entries will hit performance a little
> but I believe the entries will be regenerated if they are removed.
> While not fatal let's see if we can improve it.
>
> We can't use the class as the key as this is what's causing the memory
> leak, i.e. the $Proxy in the map also holds onto it and hence the
> class is never unreferenced.
>
> I could make this a normal map but then the $Proxy objects won't be
> garbage collected and we're back where we started.
>
> Any ideas?
>
> Simon
Re: svn commit: r828086 -
Posted by Simon Laws <si...@googlemail.com>.
Good spot Raymond.
I'm not so concerned about it being a String rather than an Integer
although I can of course fix that.
Garbage collecting the hash map entries will hit performance a little
but I believe the entries will be regenerated if they are removed.
While not fatal let's see if we can improve it.
We can't use the class as the key as this is what's causing the memory
leak, i.e. the $Proxy in the map also holds onto it and hence the
class is never unreferenced.
I could make this a normal map but then the $Proxy objects won't be
garbage collected and we're back where we started.
Any ideas?
Simon