On Nov 14, 2007, at 21:38, Jeremias Maerki wrote:
Hi Jeremias, Chris,
<jm-PropertyCache-MemLeak.diff.txt>
My proposal, incorporating the changes in Jeremias' diff, below.
To sum it up:
Only one CacheCleaner per PropertyCache, and one accompanying thread.
If, after a put(), cleanup seems to be needed *and* the thread is not
alive, lock on the cleaner, and start the thread.
If the thread is busy, I guess it suffices to continue, assuming that
the hash distribution should eventually lead some put() back to same
bucket/segment...?
As Jeremias noted, currently rehash is called for every time an
attempt to clean up fails. Maybe this needs improvement... OTOH,
rehash now becomes a bit simpler, since it does not need to take into
account interfering cleaners. Only one remaining: CacheCleaner.run()
is now synchronized on the cleaner, and rehash() itself is done
within the cleaner thread.
What I see as a possible issue though, is that there is a theoretical
limit to rehash() having any effect whatsoever. If the cache grows to
64 buckets, then the maximum number of segments that exceed the
threshold can never be greater than half the table-size... This might
be a non-issue, as this would only be triggered if the cache's size
is at least 2048 instances (not counting the elements in the buckets
that don't exceed the threshold). No problem for enums, keeps.
Strings and numbers, though?
Cheers
Andreas
Index: src/java/org/apache/fop/fo/properties/PropertyCache.java
===================================================================
--- src/java/org/apache/fop/fo/properties/PropertyCache.java
(revision 594306)
+++ src/java/org/apache/fop/fo/properties/PropertyCache.java
(working copy)
@@ -41,6 +41,9 @@
/** the table of hash-buckets */
private CacheEntry[] table = new CacheEntry[8];
+ private CacheCleaner cleaner = new CacheCleaner();
+ private Thread cleanerThread = new Thread(cleaner);
+
/* same hash function as used by java.util.HashMap */
private static int hash(Object x) {
int h = x.hashCode();
@@ -77,6 +80,10 @@
this.hash = old.hash;
}
+ boolean isCleared() {
+ return (ref == null || ref.get() == null);
+ }
+
}
/* Wrapper objects to synchronize on */
@@ -85,7 +92,7 @@
}
/*
- * Class modeling a cleanup thread.
+ * Class modeling the cleanup thread.
*
* Once run() is called, the segment is locked and the hash-bucket
* will be traversed, removing any obsolete entries.
@@ -95,50 +102,51 @@
private int hash;
- CacheCleaner(int hash) {
+ CacheCleaner() {
+ }
+
+ void init(int hash) {
this.hash = hash;
}
public void run() {
- //System.out.println("Cleaning segment " + this.segment);
- CacheSegment segment = segments[this.hash & SEGMENT_MASK];
- int oldCount;
- int newCount;
- synchronized (segment) {
- oldCount = segment.count;
- /* check first to see if another cleaner thread already
- * pushed the number of entries back below the
threshold
- * if so, return immediately
- */
- if (segment.count < (2 * table.length)) {
- return;
- }
-
- int index = this.hash & (table.length - 1);
- CacheEntry first = table[index];
- WeakReference ref;
- for (CacheEntry e = first; e != null; e = e.next) {
- ref = e.ref;
- if (ref != null && ref.get() == null) {
- /* remove obsolete entry
- /* 1. clear value, cause interference for
non-blocking get() */
- e.ref = null;
-
- /* 2. clone the segment, without the
obsolete entry */
- CacheEntry head = e.next;
- for (CacheEntry c = first; c != e; c =
c.next) {
- head = new CacheEntry(c, head);
+ synchronized (this) {
+ //System.out.println("Cleaning segment " +
this.segment);
+ CacheSegment segment = segments[this.hash &
SEGMENT_MASK];
+ int oldCount;
+ int newCount;
+ synchronized (segment) {
+ oldCount = segment.count;
+
+ int index = this.hash & (table.length - 1);
+ CacheEntry first = table[index];
+ for (CacheEntry e = first; e != null; e = e.next) {
+ if (e.isCleared()) {
+ /* remove obsolete entry
+ /* 1. clear value, cause interference
for non-blocking get() */
+ e.ref = null;
+
+ /* 2. clone the segment, without the
obsolete entry */
+ CacheEntry head = e.next;
+ for (CacheEntry c = first; c != e; c =
c.next) {
+ if (!c.isCleared()) {
+ head = new CacheEntry(c, head);
+ } else {
+ /* WeakReference cleared by the
GC? */
+ segment.count--;
+ }
+ }
+ table[index] = head;
+ segment.count--;
}
- table[index] = head;
- segment.count--;
}
+ newCount = segment.count;
}
- newCount = segment.count;
+ if (oldCount == newCount) {
+ /* cleanup had no effect, try rehashing */
+ rehash(SEGMENT_MASK);
+ }
}
- if (oldCount == newCount) {
- /* cleanup had no effect, try rehashing */
- rehash(SEGMENT_MASK);
- }
}
}
@@ -174,11 +182,14 @@
}
if (segment.count > (2 * table.length)) {
- /* launch cleanup in a separate thread,
- * so it acquires its own lock, and put()
- * can return immediately */
- Thread cleaner = new Thread(new CacheCleaner(hash));
- cleaner.start();
+ if (!cleanerThread.isAlive()) {
+ /* start cleanup thread, which acquires its own
lock,
+ * so put() can return immediately */
+ synchronized (cleaner) {
+ cleaner.init(hash);
+ cleanerThread.start();
+ }
+ }
}
}
}
@@ -267,19 +278,15 @@
CacheEntry[] newTable = new CacheEntry[newLength];
int hash, idx;
- WeakReference ref;
Object o;
newLength--;
for (int i = table.length; --i >= 0;) {
for (CacheEntry c = table[i]; c != null; c
= c.next) {
- ref = c.ref;
- if (ref != null) {
- if ((o = ref.get()) != null) {
- hash = hash(o);
- idx = hash & newLength;
- newTable[idx] = new CacheEntry
(c, newTable[idx]);
- segments[hash &
SEGMENT_MASK].count++;
- }
+ if ((o = c.ref.get()) != null) {
+ hash = hash(o);
+ idx = hash & newLength;
+ newTable[idx] = new CacheEntry(c,
newTable[idx]);
+ segments[hash & SEGMENT_MASK].count++;
}
}
}
@@ -296,6 +303,8 @@
for (int i = SEGMENT_MASK + 1; --i >= 0;) {
segments[i] = new CacheSegment();
}
+ cleanerThread.setName("FOP PropertyCache Cleaner Thread");
+ cleanerThread.setDaemon(true);
}
/**
