Sounds pretty good. About the potential issue you described: Writing
unit tests for the class can help you a long way catching potential
problems early. Might actually be worth it considering such a critical
components in the FO tree area. Just a thought.

Jeremias Maerki



On 15.11.2007 01:18:32 Andreas L Delmelle wrote:
> 
> 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);
>       }
> 
>       /**

Reply via email to