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
