[RFC] [Plugin] Fixes 100% cpu load

[Andrew Su]

----- "Dr Andrew John Hughes" <ahughes at redhat.com> wrote:

> From: "Dr Andrew John Hughes" <ahughes at redhat.com>
> To: "Andrew Haley" <aph at redhat.com>
> Cc: distro-pkg-dev at openjdk.java.net
> Sent: Thursday, September 30, 2010 8:03:29 AM GMT -05:00 US/Canada Eastern
> Subject: Re: [RFC] [Plugin] Fixes 100% cpu load
>
> On 10:07 Thu 30 Sep     , Andrew Haley wrote:
> > On 09/28/2010 10:14 PM, Andrew Su wrote:
> > > Update:
> > > -Changed to use ConcurrentHashMap.
> > > -Used suggestion of putIfAbsent instead of locking.
> > > 
> > > Cheers,
> > > --Andrew
> > > 
> > > Attached:
> > > Modified version of file.
> > > Patch file.
> > 
> > One small thing: appLock is a collection of locks, so should be
> called
> > appLocks.
> > 
> 
> Good point.
> 
> > The code
> > 
> >   appLock.get(identifier).notifyAll();
> > 
> > is weird: isn't that lock already held?
> > 
> 
> Well, notifyAll requires that the calling thread holds the monitor so
> calling it outside an appLock.get(identifier) synchronised block
> would
> throw an IllegalMonitorStateException.
> 
> The real problem (and what I think you may actually be referring to)
> is
> that the wait() call is never going to be made, because if an init
> message
> is being processed, the lock is already held and another thread won't
> be
> able to enter the else block.  I don't see anywhere in the init
> processing
> where the lock is released.
Yes, but the idea was to wait until the message is done being handled before doing another message. 
Lock is released the moment you leave the synchronization block.
We could put the notifyAll() into the finally block of the try catch, instead and then avoid having to put a timeout for the calls to wait().
The wait() call is by chance we get a message other than destroy and the applet is not initialized, this way another thread has the chance of processing the init of that applet.
wait is woken up every 5 seconds (Oops, I put 6 zeroes too many, fixed in v1) even if no notify is called.

After considering other alternatives, we can get rid of the wait and notify, but instead just do some checking at the end of initializing the applet. If we had ever received a destroy message while we were initializing, we can simply call destroy on it (v2). 

> 
> > The way locking is being done here makes me very nervous.  The
> locked
> > region is very long, and the interaction between threads and locks
> is
> > complex.
> > 
> > I wonder if the additional synchronization is patching around a
> design
> > problem.
> > 
>From before the recent changes to the plugin there was the problem with the messages being processed out of order and causing applets resize inappropriately. It was changed to collect all the messages that affect init to be done in one block. (Refer to changeset 2254 c764b38139a5)

We still want to have the initializing of the applet atomic, it would be bad to have it get interrupted in the process of starting up. Using either the current patch or the alternative one will produce the similar results AFAICT.

Note: The alternative patch is similar to the way the plugin handles the messages prior to changeset 2254.

Attachments:
20100930_destroy_deadlock_v1.patch:
--Uses 1 synchronization block
--Uses wait() and notifyAll()
---notifyAll() is moved to the finally block of the try catch, so it will always execute.

20100930_destroy_deadlock_v2.patch:
--Uses 2 synchronization blocks
--Does not rely on wait and notify
--Similar to original implementation
--Side-effect: Fixes destroyed applet opening in new windows (This can be fixed in v1 with minor changes, better to keep as separate changesets)

Questions? Comments? Concerns?

--Andrew

> 
> Agreed.  I've already gone over this twice and still managed to miss
> the
> whole wait/notifyAll part.  This is why I'd like to see some more
> information
> on the general architecture we're dealing with.
> 
> If it's the case that each message needs to be processed sequentially,
> then
> the simplest solution may be to allocate a thread per identifier and
> give it
> a work queue of messages which are then processed one at a time, in
> order.
> But this may be complete nonsense, as I don't know what the overall
> design is.
It would probably better to keep it as x# of threads handling the messages. 
This will keep the number of threads we need to keep track of minimal. 
Looking at the file PluginMessageConsumer.java, there it shows 5 threads being created to handle each of the different messages (it does not seem to do handle it that way though). 

> 
> > Andrew.
> 
> -- 
> Andrew :)
> 
> Free Java Software Engineer
> Red Hat, Inc. (http://www.redhat.com)
> 
> Support Free Java!
> Contribute to GNU Classpath and the OpenJDK
> http://www.gnu.org/software/classpath
> http://openjdk.java.net
> PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
> Fingerprint = F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20100930_destroy_deadlock_v2.patch
Type: text/x-patch
Size: 11334 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20100930/f25ebba2/20100930_destroy_deadlock_v2.patch 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20100930_destroy_deadlock_v1.patch
Type: text/x-patch
Size: 10085 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20100930/f25ebba2/20100930_destroy_deadlock_v1.patch