From 91d7a329d4362fc65bc78123fb9f1b9b6b4cd7e4 Mon Sep 17 00:00:00 2001 From: Trustin Lee Date: Fri, 13 Feb 2009 06:19:10 +0000 Subject: [PATCH] * Renamed Concurrent*Weak*HashMap to Concurrent*WeakKey*HashMap to avoid confusion * Added some FIXMEs related with potential memory leak --- .../org/jboss/netty/channel/ChannelLocal.java | 4 +- .../channel/group/ChannelGroupFactory.java | 1 + .../netty/channel/local/LocalAddress.java | 5 ++- .../execution/DefaultObjectSizeEstimator.java | 4 +- ... => ConcurrentIdentityWeakKeyHashMap.java} | 44 +++++++++---------- ...Map.java => ConcurrentWeakKeyHashMap.java} | 44 +++++++++---------- 6 files changed, 52 insertions(+), 50 deletions(-) rename src/main/java/org/jboss/netty/util/{ConcurrentIdentityWeakHashMap.java => ConcurrentIdentityWeakKeyHashMap.java} (97%) rename src/main/java/org/jboss/netty/util/{ConcurrentWeakHashMap.java => ConcurrentWeakKeyHashMap.java} (97%) diff --git a/src/main/java/org/jboss/netty/channel/ChannelLocal.java b/src/main/java/org/jboss/netty/channel/ChannelLocal.java index 5b48a7639c..23dd11f4bd 100644 --- a/src/main/java/org/jboss/netty/channel/ChannelLocal.java +++ b/src/main/java/org/jboss/netty/channel/ChannelLocal.java @@ -24,7 +24,7 @@ package org.jboss.netty.channel; import java.util.concurrent.ConcurrentMap; -import org.jboss.netty.util.ConcurrentIdentityWeakHashMap; +import org.jboss.netty.util.ConcurrentIdentityWeakKeyHashMap; /** * @author The Netty Project (netty-dev@lists.jboss.org) @@ -35,7 +35,7 @@ import org.jboss.netty.util.ConcurrentIdentityWeakHashMap; */ public class ChannelLocal { private final ConcurrentMap map = - new ConcurrentIdentityWeakHashMap(); + new ConcurrentIdentityWeakKeyHashMap(); /** * Creates a {@link Channel} local variable. diff --git a/src/main/java/org/jboss/netty/channel/group/ChannelGroupFactory.java b/src/main/java/org/jboss/netty/channel/group/ChannelGroupFactory.java index bec50af73d..24bbd60b4a 100644 --- a/src/main/java/org/jboss/netty/channel/group/ChannelGroupFactory.java +++ b/src/main/java/org/jboss/netty/channel/group/ChannelGroupFactory.java @@ -37,6 +37,7 @@ import org.jboss.netty.util.ConcurrentHashMap; */ public class ChannelGroupFactory { + // FIXME: Memory leak - use ConcurrentWeakValueHashMap private static final ConcurrentMap groups = new ConcurrentHashMap(); diff --git a/src/main/java/org/jboss/netty/channel/local/LocalAddress.java b/src/main/java/org/jboss/netty/channel/local/LocalAddress.java index f10ce57893..a402e72bcb 100644 --- a/src/main/java/org/jboss/netty/channel/local/LocalAddress.java +++ b/src/main/java/org/jboss/netty/channel/local/LocalAddress.java @@ -26,7 +26,7 @@ import java.util.concurrent.ConcurrentMap; import java.util.concurrent.atomic.AtomicInteger; import org.jboss.netty.channel.ChannelException; -import org.jboss.netty.util.ConcurrentWeakHashMap; +import org.jboss.netty.util.ConcurrentHashMap; /** * @author The Netty Project (netty-dev@lists.jboss.org) @@ -37,8 +37,9 @@ import org.jboss.netty.util.ConcurrentWeakHashMap; public final class LocalAddress extends SocketAddress implements Comparable { private static final long serialVersionUID = -3601961747680808645L; + // FIXME: Memory leak - use ConcurrentWeakValueHashMap private static final ConcurrentMap addresses = - new ConcurrentWeakHashMap(); + new ConcurrentHashMap(); private static final AtomicInteger nextEphemeralPort = new AtomicInteger(); diff --git a/src/main/java/org/jboss/netty/handler/execution/DefaultObjectSizeEstimator.java b/src/main/java/org/jboss/netty/handler/execution/DefaultObjectSizeEstimator.java index 1db76e2c61..88b6244873 100644 --- a/src/main/java/org/jboss/netty/handler/execution/DefaultObjectSizeEstimator.java +++ b/src/main/java/org/jboss/netty/handler/execution/DefaultObjectSizeEstimator.java @@ -31,7 +31,7 @@ import java.util.concurrent.ConcurrentMap; import org.jboss.netty.buffer.ChannelBuffer; import org.jboss.netty.channel.MessageEvent; -import org.jboss.netty.util.ConcurrentHashMap; +import org.jboss.netty.util.ConcurrentIdentityWeakKeyHashMap; /** * The default {@link ObjectSizeEstimator} implementation for general purpose. @@ -45,7 +45,7 @@ import org.jboss.netty.util.ConcurrentHashMap; public class DefaultObjectSizeEstimator implements ObjectSizeEstimator { private final ConcurrentMap, Integer> class2size = - new ConcurrentHashMap, Integer>(); + new ConcurrentIdentityWeakKeyHashMap, Integer>(); /** * Creates a new instance. diff --git a/src/main/java/org/jboss/netty/util/ConcurrentIdentityWeakHashMap.java b/src/main/java/org/jboss/netty/util/ConcurrentIdentityWeakKeyHashMap.java similarity index 97% rename from src/main/java/org/jboss/netty/util/ConcurrentIdentityWeakHashMap.java rename to src/main/java/org/jboss/netty/util/ConcurrentIdentityWeakKeyHashMap.java index efed7ac157..f6fc198d63 100644 --- a/src/main/java/org/jboss/netty/util/ConcurrentIdentityWeakHashMap.java +++ b/src/main/java/org/jboss/netty/util/ConcurrentIdentityWeakKeyHashMap.java @@ -58,7 +58,7 @@ import java.util.concurrent.locks.ReentrantLock; * @param the type of keys maintained by this map * @param the type of mapped values */ -public final class ConcurrentIdentityWeakHashMap extends AbstractMap implements ConcurrentMap { +public final class ConcurrentIdentityWeakKeyHashMap extends AbstractMap implements ConcurrentMap { /* * The basic strategy is to subdivide the table among Segments, @@ -672,7 +672,7 @@ public final class ConcurrentIdentityWeakHashMap extends AbstractMap * the load factor or concurrencyLevel are * nonpositive. */ - public ConcurrentIdentityWeakHashMap( + public ConcurrentIdentityWeakKeyHashMap( int initialCapacity, float loadFactor, int concurrencyLevel) { if (!(loadFactor > 0) || initialCapacity < 0 || concurrencyLevel <= 0) { throw new IllegalArgumentException(); @@ -724,7 +724,7 @@ public final class ConcurrentIdentityWeakHashMap extends AbstractMap * negative or the load factor is * nonpositive */ - public ConcurrentIdentityWeakHashMap(int initialCapacity, float loadFactor) { + public ConcurrentIdentityWeakKeyHashMap(int initialCapacity, float loadFactor) { this(initialCapacity, loadFactor, DEFAULT_CONCURRENCY_LEVEL); } @@ -738,7 +738,7 @@ public final class ConcurrentIdentityWeakHashMap extends AbstractMap * @throws IllegalArgumentException if the initial capacity of elements is * negative. */ - public ConcurrentIdentityWeakHashMap(int initialCapacity) { + public ConcurrentIdentityWeakKeyHashMap(int initialCapacity) { this(initialCapacity, DEFAULT_LOAD_FACTOR, DEFAULT_CONCURRENCY_LEVEL); } @@ -747,7 +747,7 @@ public final class ConcurrentIdentityWeakHashMap extends AbstractMap * types (weak keys, strong values), default load factor (0.75) and * concurrencyLevel (16). */ - public ConcurrentIdentityWeakHashMap() { + public ConcurrentIdentityWeakKeyHashMap() { this(DEFAULT_INITIAL_CAPACITY, DEFAULT_LOAD_FACTOR, DEFAULT_CONCURRENCY_LEVEL); } @@ -759,7 +759,7 @@ public final class ConcurrentIdentityWeakHashMap extends AbstractMap * * @param m the map */ - public ConcurrentIdentityWeakHashMap(Map m) { + public ConcurrentIdentityWeakKeyHashMap(Map m) { this(Math.max((int) (m.size() / DEFAULT_LOAD_FACTOR) + 1, DEFAULT_INITIAL_CAPACITY), DEFAULT_LOAD_FACTOR, DEFAULT_CONCURRENCY_LEVEL); @@ -1273,7 +1273,7 @@ public final class ConcurrentIdentityWeakHashMap extends AbstractMap if (lastReturned == null) { throw new IllegalStateException(); } - ConcurrentIdentityWeakHashMap.this.remove(currentKey); + ConcurrentIdentityWeakKeyHashMap.this.remove(currentKey); lastReturned = null; } } @@ -1391,7 +1391,7 @@ public final class ConcurrentIdentityWeakHashMap extends AbstractMap throw new NullPointerException(); } V v = super.setValue(value); - ConcurrentIdentityWeakHashMap.this.put(getKey(), value); + ConcurrentIdentityWeakKeyHashMap.this.put(getKey(), value); return v; } @@ -1414,28 +1414,28 @@ public final class ConcurrentIdentityWeakHashMap extends AbstractMap @Override public int size() { - return ConcurrentIdentityWeakHashMap.this.size(); + return ConcurrentIdentityWeakKeyHashMap.this.size(); } @Override public boolean isEmpty() { - return ConcurrentIdentityWeakHashMap.this.isEmpty(); + return ConcurrentIdentityWeakKeyHashMap.this.isEmpty(); } @Override public boolean contains(Object o) { - return ConcurrentIdentityWeakHashMap.this.containsKey(o); + return ConcurrentIdentityWeakKeyHashMap.this.containsKey(o); } @Override public boolean remove(Object o) { - return ConcurrentIdentityWeakHashMap.this.remove(o) != null; + return ConcurrentIdentityWeakKeyHashMap.this.remove(o) != null; } @Override public void clear() { - ConcurrentIdentityWeakHashMap.this.clear(); + ConcurrentIdentityWeakKeyHashMap.this.clear(); } } @@ -1447,22 +1447,22 @@ public final class ConcurrentIdentityWeakHashMap extends AbstractMap @Override public int size() { - return ConcurrentIdentityWeakHashMap.this.size(); + return ConcurrentIdentityWeakKeyHashMap.this.size(); } @Override public boolean isEmpty() { - return ConcurrentIdentityWeakHashMap.this.isEmpty(); + return ConcurrentIdentityWeakKeyHashMap.this.isEmpty(); } @Override public boolean contains(Object o) { - return ConcurrentIdentityWeakHashMap.this.containsValue(o); + return ConcurrentIdentityWeakKeyHashMap.this.containsValue(o); } @Override public void clear() { - ConcurrentIdentityWeakHashMap.this.clear(); + ConcurrentIdentityWeakKeyHashMap.this.clear(); } } @@ -1478,7 +1478,7 @@ public final class ConcurrentIdentityWeakHashMap extends AbstractMap return false; } Map.Entry e = (Map.Entry) o; - V v = ConcurrentIdentityWeakHashMap.this.get(e.getKey()); + V v = ConcurrentIdentityWeakKeyHashMap.this.get(e.getKey()); return v != null && v.equals(e.getValue()); } @@ -1488,22 +1488,22 @@ public final class ConcurrentIdentityWeakHashMap extends AbstractMap return false; } Map.Entry e = (Map.Entry) o; - return ConcurrentIdentityWeakHashMap.this.remove(e.getKey(), e.getValue()); + return ConcurrentIdentityWeakKeyHashMap.this.remove(e.getKey(), e.getValue()); } @Override public int size() { - return ConcurrentIdentityWeakHashMap.this.size(); + return ConcurrentIdentityWeakKeyHashMap.this.size(); } @Override public boolean isEmpty() { - return ConcurrentIdentityWeakHashMap.this.isEmpty(); + return ConcurrentIdentityWeakKeyHashMap.this.isEmpty(); } @Override public void clear() { - ConcurrentIdentityWeakHashMap.this.clear(); + ConcurrentIdentityWeakKeyHashMap.this.clear(); } } } diff --git a/src/main/java/org/jboss/netty/util/ConcurrentWeakHashMap.java b/src/main/java/org/jboss/netty/util/ConcurrentWeakKeyHashMap.java similarity index 97% rename from src/main/java/org/jboss/netty/util/ConcurrentWeakHashMap.java rename to src/main/java/org/jboss/netty/util/ConcurrentWeakKeyHashMap.java index b569706e36..f166d1e5e0 100644 --- a/src/main/java/org/jboss/netty/util/ConcurrentWeakHashMap.java +++ b/src/main/java/org/jboss/netty/util/ConcurrentWeakKeyHashMap.java @@ -58,7 +58,7 @@ import java.util.concurrent.locks.ReentrantLock; * @param the type of keys maintained by this map * @param the type of mapped values */ -public final class ConcurrentWeakHashMap extends AbstractMap implements ConcurrentMap { +public final class ConcurrentWeakKeyHashMap extends AbstractMap implements ConcurrentMap { /* * The basic strategy is to subdivide the table among Segments, @@ -672,7 +672,7 @@ public final class ConcurrentWeakHashMap extends AbstractMap impleme * the load factor or concurrencyLevel are * nonpositive. */ - public ConcurrentWeakHashMap( + public ConcurrentWeakKeyHashMap( int initialCapacity, float loadFactor, int concurrencyLevel) { if (!(loadFactor > 0) || initialCapacity < 0 || concurrencyLevel <= 0) { throw new IllegalArgumentException(); @@ -724,7 +724,7 @@ public final class ConcurrentWeakHashMap extends AbstractMap impleme * negative or the load factor is * nonpositive */ - public ConcurrentWeakHashMap(int initialCapacity, float loadFactor) { + public ConcurrentWeakKeyHashMap(int initialCapacity, float loadFactor) { this(initialCapacity, loadFactor, DEFAULT_CONCURRENCY_LEVEL); } @@ -738,7 +738,7 @@ public final class ConcurrentWeakHashMap extends AbstractMap impleme * @throws IllegalArgumentException if the initial capacity of elements is * negative. */ - public ConcurrentWeakHashMap(int initialCapacity) { + public ConcurrentWeakKeyHashMap(int initialCapacity) { this(initialCapacity, DEFAULT_LOAD_FACTOR, DEFAULT_CONCURRENCY_LEVEL); } @@ -747,7 +747,7 @@ public final class ConcurrentWeakHashMap extends AbstractMap impleme * types (weak keys, strong values), default load factor (0.75) and * concurrencyLevel (16). */ - public ConcurrentWeakHashMap() { + public ConcurrentWeakKeyHashMap() { this(DEFAULT_INITIAL_CAPACITY, DEFAULT_LOAD_FACTOR, DEFAULT_CONCURRENCY_LEVEL); } @@ -759,7 +759,7 @@ public final class ConcurrentWeakHashMap extends AbstractMap impleme * * @param m the map */ - public ConcurrentWeakHashMap(Map m) { + public ConcurrentWeakKeyHashMap(Map m) { this(Math.max((int) (m.size() / DEFAULT_LOAD_FACTOR) + 1, DEFAULT_INITIAL_CAPACITY), DEFAULT_LOAD_FACTOR, DEFAULT_CONCURRENCY_LEVEL); @@ -1273,7 +1273,7 @@ public final class ConcurrentWeakHashMap extends AbstractMap impleme if (lastReturned == null) { throw new IllegalStateException(); } - ConcurrentWeakHashMap.this.remove(currentKey); + ConcurrentWeakKeyHashMap.this.remove(currentKey); lastReturned = null; } } @@ -1391,7 +1391,7 @@ public final class ConcurrentWeakHashMap extends AbstractMap impleme throw new NullPointerException(); } V v = super.setValue(value); - ConcurrentWeakHashMap.this.put(getKey(), value); + ConcurrentWeakKeyHashMap.this.put(getKey(), value); return v; } @@ -1414,28 +1414,28 @@ public final class ConcurrentWeakHashMap extends AbstractMap impleme @Override public int size() { - return ConcurrentWeakHashMap.this.size(); + return ConcurrentWeakKeyHashMap.this.size(); } @Override public boolean isEmpty() { - return ConcurrentWeakHashMap.this.isEmpty(); + return ConcurrentWeakKeyHashMap.this.isEmpty(); } @Override public boolean contains(Object o) { - return ConcurrentWeakHashMap.this.containsKey(o); + return ConcurrentWeakKeyHashMap.this.containsKey(o); } @Override public boolean remove(Object o) { - return ConcurrentWeakHashMap.this.remove(o) != null; + return ConcurrentWeakKeyHashMap.this.remove(o) != null; } @Override public void clear() { - ConcurrentWeakHashMap.this.clear(); + ConcurrentWeakKeyHashMap.this.clear(); } } @@ -1447,22 +1447,22 @@ public final class ConcurrentWeakHashMap extends AbstractMap impleme @Override public int size() { - return ConcurrentWeakHashMap.this.size(); + return ConcurrentWeakKeyHashMap.this.size(); } @Override public boolean isEmpty() { - return ConcurrentWeakHashMap.this.isEmpty(); + return ConcurrentWeakKeyHashMap.this.isEmpty(); } @Override public boolean contains(Object o) { - return ConcurrentWeakHashMap.this.containsValue(o); + return ConcurrentWeakKeyHashMap.this.containsValue(o); } @Override public void clear() { - ConcurrentWeakHashMap.this.clear(); + ConcurrentWeakKeyHashMap.this.clear(); } } @@ -1478,7 +1478,7 @@ public final class ConcurrentWeakHashMap extends AbstractMap impleme return false; } Map.Entry e = (Map.Entry) o; - V v = ConcurrentWeakHashMap.this.get(e.getKey()); + V v = ConcurrentWeakKeyHashMap.this.get(e.getKey()); return v != null && v.equals(e.getValue()); } @@ -1488,22 +1488,22 @@ public final class ConcurrentWeakHashMap extends AbstractMap impleme return false; } Map.Entry e = (Map.Entry) o; - return ConcurrentWeakHashMap.this.remove(e.getKey(), e.getValue()); + return ConcurrentWeakKeyHashMap.this.remove(e.getKey(), e.getValue()); } @Override public int size() { - return ConcurrentWeakHashMap.this.size(); + return ConcurrentWeakKeyHashMap.this.size(); } @Override public boolean isEmpty() { - return ConcurrentWeakHashMap.this.isEmpty(); + return ConcurrentWeakKeyHashMap.this.isEmpty(); } @Override public void clear() { - ConcurrentWeakHashMap.this.clear(); + ConcurrentWeakKeyHashMap.this.clear(); } } }