geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Matt Hogstrom <m...@hogstrom.org>
Subject Re: svn commit: r409112 - EJB Performance Improvement of 6x (did I miss something ?)!
Date Wed, 24 May 2006 19:41:42 GMT
According to the ConcurrentReaderHashMap doc a Null value is returned if this value was added
and 
there are no dups.  If there was a dup (meaning you did a get and on null a put in the example

below) then you'll get non-null value in return (meaning you now have more than one).  If
its now 
positive then my put caused the cache to grow (simple race condition) so I remove it.  Subsequent

get's will succeed and no more puts will be done.  I think that net's out to a single entry
and the 
additional code is simply managing a race condition for the first to add to the Map.

Am I mis interpreting the code?  I ran a test for 30 minutes and did not see the heap growing
but 
could add some diagnostic code to dump the count on every 1000n requests.

Matt

Dain Sundstrom wrote:
> 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