Re: Relieving some ClassLoader contention in PreferredClassProvider

On Tue, Jan 02, 2007 at 01:58:28PM -0600, Gregg Wonderly wrote:
>>> I brought this up on the concurrency-interest mailing list,
>>> because ConcurrentHashMap.putIfAbsent() is randomly returning the
>>> wrong value.
>>
>> Are you saying you think the problem is in URL or
>> ConcurrentHashMap, or both?
>
> I'm not sure yet because I haven't been able to spend time on
> finding the exact cause.  The only thing that I do know, is that
>
> ConcurrentHashmap map;
> synchronized( map ) {
>        Object fut = map.get(key);
>        if( fut == null ) {
>                map.put( key, newfut );
>                fut = newfut;
>        }
> }
>
> worked differently than
>
> fut = map.putIfAbsent( key, newfut );
>
> based on logging that I put around these two blocks in my initial
> evaluation of some ClassCastException traces comming out at random
> moments when my (too short) ServiceRegistrar.notify() lease would
> expire and I'd renew the notify in a background thread.
>
> The synchronized block changed the logging output to indicate that
> the right entry was in the map after the block based on the
> information shown inside of the synchronzied block, but before the
> get/put pair were executed.
>
> The putIfAbsent call, without any synchronization around the logging
> and the putIfAbsent call would sometimes indicate in a
> map.toString() list that an entry was present and putIfAbsent() did
> not see it.

I had been meaning to reply to your earlier JINI-USERS post[1] and/or
your concurrency-interest post[2] about this modification in December,
but time ran out (Happy New Year).

Is your modified PreferredClassProvider implementation still like you
presented in this blog entry (if it's not, then the rest of this
message might not apply):

        http://www.artima.com/weblogs/viewpost.jsp?thread=182203

I can't offhand explain why there would be a difference between using
putIfAbesnt and the simple alternative synchronized snippet you
presented below, but I do see how the behavior you reported on
concurrency-interest can happen with the modified implementation in
the blog entry: the loaderFutures.remove in the first synchronized
block can remove a mapping in more cases than you seem to be expecting
it to-- i.e. not just when the weak reference to the loader has been
cleared.  Consider this sequence of events:

- An invocation of this method in one thread with some key completes
  the first synchronized block having found no entry in loaderTable for
  the key.  It creates a FutureTask and passes the result to
  loaderFutures.putIFAbesnt, which returns null as expected.  It will
  then execute the FutureTask, but that hasn't happened yet.

- An invocation of this method in another thread with the same key
  enters the first synchronized block, discovers no entry in
  loaderTable (because the first thread's FutureTask hasn't executed
  yet), and thus invokes loaderFutures.remove with the key.  Therefore,
  the subsequent loaderFutures.putIfAbsent will again return null.

Some other comments on the code in the blog entry:

The code comments regarding the "curLoader" variable suggest that you
expect a guarantee that its value be considered strongly reachable
from the variable's setting until the completion of the method.
Hotspot's GC is (at least since JDK 5.0) too clever for that
expectation to hold-- it can realize that a variable is not used after
a certain point in a method (in this case, it is actually not used at
all) and thus not consider the variable reachable after that point.
This RMI distributed garbage collection bug was an example of such a
flawed expectation (in that case, we had expected that argument values
on multiple methods' call frames would be sufficient to guarantee
reachability, but in fact they are not if none of those methods can
possibly use those arguments again):

        http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6181943

But I don't see why you even need to worry about that kind of issue
here-- if the loader value is known in the first synchronized block,
the method should just return it right there.

Why bother executing the task in a thread pool, instead of just using
the current thread, which has nothing better to do at that point, and
executes it directly in the unmodified implementation?

More generally, I think that the improvements of

(A) constructing values without locking the whole table
(B) using ConcurrentHashMap

are conceptually separable and probably should be thought of as such
here-- my understanding is that your primary goal was (A), so I might
suggest starting with just that, instead of (B) too.  In particular,
I'm not sure what value ConcurrentHashMap is providing in your current
implementation, given that you are still fully synchronizing on the
original loaderTable HashMap at least once for every lookup as well.
I think that an ultimate solution involving both (A) and (B) would
just have one ConcurrentHashMap.

And I think that the challenge with using ConcurrentHashMap here is in
correctly dealing with all of the weak references, in both the keys
and the values (see above problem).  For comparison, I might suggest
looking at the static "localDescs" table of java.io.ObjectStreamClass:
prior to JDK 6, it was a regular synchronized HashMap that used a
custom internal "EntryFuture" class to accomplish (A); for JDK 6, it
was changed to a ConcurrentHashMap for this RFE (which was encouraged
by Doug Lea):

        http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=5056445

The keys (Class objects) need to be referenced weakly and the values
(java.io.ObjectStreamClass objects) need to be referenced softly, both
so that the keys aren't inappropriately pinned[3] and so that this
serialization-related data isn't cached for the lifetime of a Class
that has been serialized.  Maintaining these reachability properties
correctly with a ConcurrentHashMap (i.e. map that can't be externally
synchronized across multiple operations) was somewhat tricky: I seem
to recall a few iterations of code review before we arrived at
something that (I think) is correct-- if I recall correctly, earlier
iterations involved more substantial changes, use of j.u.c.Future
objects, and more expressive use of generics, but we encountered
various subtle problems with the intended reachability characteristics
and ultimately ended up with a smaller change (from JDK 5.0) that
seemed correct.

At the Core Libraries BOF at last year's JavaOne, there was mention of
the possibility of a Map implementation framework that more generally
supported various permutations of reachability for keys and values
(and presumably concurrency properties)-- something like that would
certainly have been useful there.

-- Peter

[1] http://archives.java.sun.com/cgi-bin/wa?A2=ind0610&L=jini-users&P=31518

[2] http://altair.cs.oswego.edu/mailman/private/concurrency-interest/2006-December/003379.html

[3] Would anyone like to sell me their JDC/SDN votes for this bug?
        http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4429536
(I'm only barely joking.)

--------------------------------------------------------------------------
Getting Started:     http://www.jini.org/wiki/Category:Getting_Started
Community Web Site:  http://jini.org
jini-users Archive:  http://archives.java.sun.com/archives/jini-users.html
Unsubscribing:       email "signoff JINI-USERS"  to [email protected]