Tim... you seem to be agreeing with me, then, that
this does have something to do with the Application, and you are also
allowing for some redundancy in calculation and memory use, so, again,
why not just store it in the Application's Context, where all this is
taken care of automatically? I think Jerome's first instinct here may
have been the most straightforward.
Barring that, how about calling the clearCache in the Application's
release(), rather than stop()?
-Tal
Tim Peierls wrote:
I wouldn't recommend RRWL for this.
Since it's harmless to recompute the AnnotationInfos, I think
just having an AnnotationUtils.clearCache method would work well. Call
it whenever an Application stops, say. It's worth a little redundant
computation to avoid having to deal with shared locks and liveness.
--tim
On Thu, Apr 9, 2009 at 2:22 PM, Tal Liron <tal.li...@threecrickets.com>
wrote:
Oh, I didn't mean synchronizing the whole map.
I
actually didn't mean much beyond suggesting that we brainstorm a way to
use WeakReferences as keys in this particular case. I still don't have
a complete solution in mind.
Another idea: Perhaps we can use a ReentrantReadWrite lock around a
simple WeakHashMap? Since this is a "99% read" situation, most requests
won't need to acquire locks.
(ConcurrentHashMap is actually a very
complicated and delicately tuned class, and it would be anything but
trivial for us to try to re-implement it using WeakReferences.)
I know there are generic 3rd party solutions to this, but I know we'd
all prefer to use what Java 5 offers us right now. I'm sure we can
cobble together something that performs well!
-Tal
Tim Peierls wrote:
Did you read my subsequent e-mail that
talked about
opening an issue to make this more robust?
There is currently no standard concurrent map with weak
keys,
although MapMaker in Google Collections provides this and much more. An
effort to put something like this into Java 7 is under way, but there's
no telling whether it will actually happen.
Wrapping a WeakHashMap with synchronization is likely to be
a
performance bottleneck: every request has to grab a global lock.
--tim
On Wed, Apr 8, 2009 at 2:06 PM, Tal
Liron <tal.li...@threecrickets.com>
wrote:
Hmm.
I disagree that this has nothing to do with the Application.
For example, if an Application is unloaded, this cached information
will remain in the static field. In fact, there is no mechanism in
place right now clean this cache, and in dynamic environments (possibly
OSGi) it may accumulate "cruft." Not a huge problem, but this is the
stuff memory leaks are made of...
Here's
a thought: can we use a synchronized WeakHashMap here? That way, the
AnnotationInfo would be discarded when the class is unloaded.
-Tal
Jerome Louvel wrote:
Hi
all,
Beautiful!
This new solution is available in SVN
trunk.
Thanks Dave for
the report and Tim for the clever fix!
Why involve Context at all? The AnnotationInfo associated with a
Class<? extends UniformResource> does not depend on Context. You
could just add a method to AnnotationUtils:
public static AnnotationInfo
getAnnotationDescriptor(Class<?
extends UniformResource > resourceClass) {
AnnotationInfo result = cache.get(resourceClass);
if (result == null) {
result = computeAnnotationDescriptor(resourceClass); //
use
code
from existing getAnnotationDescriptors for this
AnnotationInfo prev = cache.putIfAbsent(resourceClass,
result);
if (prev != null) result = prev;
}
return result;
}
private static final ConcurrentMap<Class<? extends
Resource>, AnnotationInfo> cache =
new ConcurrentHashMap<Class<? extends
Resource>,
AnnotationInfo>();
Then you don't need getAnnotationDescriptors. You
probably
want
a way to clear cache entries if a class is reloaded, say.
You could get more fancy, with Futures and such, but I
think
that would be wasted effort.
--tim
On Tue, Apr 7, 2009 at 1:20 PM, Tal
Liron <tal.li...@threecrickets.com>
wrote:
Ah, I can see your point now.
I would recommend, then, that a Context would have two separate
ConcurrentHashMaps, something like this:
1. Context.getConfiguration() -- this would be for the user
2. Context.getImplementationCache() -- this would be for things like
AnnotationUtils, and the user should not normally look at this, except
perhaps for debugging
-Tal
David Fogel wrote:
Hi Tal-
I think what you suggested about appending a classloader id (perhaps
System.identityHashCode(cl) ?) would work.
Regarding the Restlet Context and this cache being "cruft": You say
the Context is a reasonable place for information that applies to the
Application, but this doesn't have annotation cache doesn't have
anything to do with my Application, it's related to a hidden
_implementation_ detail. As an end-user, we're not meant to be using
this annotion info cache directly. Yet the Context, as a very
prominent and unavoidable part of the Restlet public API is meant to
be used and understood. That's why I think it isn't a good place to
stash implementation-related data like this annotation stuff.
-Dave
------------------------------------------------------
http://restlet.tigris.org/ds/viewMessage.do?dsForumId=4447&dsMessageId=1572499
|
