geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dain Sundstrom <d...@iq80.com>
Subject Re: svn commit: r409112 - EJB Performance Improvement of 6x (did I miss something ?)!
Date Wed, 24 May 2006 17:05:38 GMT
On May 24, 2006, at 3:30 AM, Matt Hogstrom wrote:

>> +    private String getNormalizedCodebase(String codebase)
>> +            throws MalformedURLException {
>> +        String cachedCodebase = (String)cachedCodebases.get 
>> (codebase);
>> +        if (cachedCodebase != null)
>> +            return cachedCodebase;
>> +
>> +        String normalizedCodebase = normalizeCodebase(codebase);
>> +        String oldValue = (String)cachedCodebases.put(codebase,  
>> normalizedCodebase);
>> +
>> +        // If there was a previous value remove the one we just  
>> added to make sure the
>> +        // cache doesn't grow.
>> +        if (oldValue != null) {
>> +            cachedCodebases.remove(codebase);
>> +        }
>> +        return normalizedCodebase;  // We can use the oldValue
>> +    }

I don't get what you are trying to do here.  It appears that you add  
the value but if the map already contained the value you turn around  
and remove it.  I don't see how that stops the cache from growing.   
If you want to prevent the cache from growing I think you need  
something like this:

     if (cache.size < CACHE_LIMIT) {
         cachedCodeBases.put(codebase, normalizedCodebase);
     }

That isn't absolutely safe but is most likely good enough.  The  
problem is once the cache is full, you will never add another value.   
Alternatively, you could drop something from the cache once it is  
full, but you would have to figure out how to do it randomly since,  
you don't have any data the entries.

You may want to consider switching the implementation to use a  
LinkedHashMap with a size limit, so old values are pushed out.  The  
problem with a LinkedHashMap is you need to synchronize access.  This  
shouldn't be a big problem as long as you keep the synchronized  
blocks small.  I'd synchronized, check if the value exists, and if it  
didn't add a future object to the map.  Then you exist the  
synchronized block and call get() on the future object.  This causes  
the expensive operation normalizedCodebase(codebase) to happen  
outside of the main synchronize block.

Regardless, I'm +1 to this patch because it will work as it stands,  
although with a small and very slow memory leak of one string per  
class loader, and it is way better then what we had before which was  
nothing :)

-dain

Mime
View raw message