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
