[RFC] [Plugin] Fixes 100% cpu load
Andrew Su
asu at redhat.com
Tue Sep 28 13:28:49 PDT 2010
Updated the patch:
Now uses HashMap instead of Hashtable.
-Synchronization that Hashtable gives is not required.
-Maybe use of WeakHashMap would be better. Comments?
--Reason: The Map would be increasing in size and taking up memory each time a new applet is loaded.
-- It would clear the key value pair for the map when it's no longer being referenced by any threads.
-- This is also true for the HashMap applets and HashMap status?
-Fixed tabs to spaces, where 4 space per tab.
First synchronization block:
-No change to this, required to ensure that we will only have one unique lock per identifier.
--If we changed to WeakHashMap, these locks will be destroyed when they are no longer needed.
Second synchronization block:
-No change either.
-It is needed to be synchronized to prevent the handling of destroy while in the middle of initializing.
--This also means we could possibly remove the while loop which waits for applet to finish initializing. No longer need to worry about applet's status changing while initializing.
Third synchronization block:
-No change either.
-Using synchronization here will let us put the thread to sleep if we get a message other than destroy and the applet is not initialized. Sleep time of 5 seconds.
-The while loop in here now checks the extra condition if it's destroy message we're getting.
--If so we will process this then end there.
--Otherwise, proceed as normal.
-Moved the handling of other messages back into this block
--Reason by example:
---Applet initialized.
---Thread gets kicked off before call to handling "GetJavaObject".
---Handles destroy message.
---Resumes handling "GetJavaObject" -> loop forever.
Questions? Comments? Concerns?
--Andrew
----- "Andrew Su" <asu at redhat.com> wrote:
> From: "Andrew Su" <asu at redhat.com>
> To: "Dr Andrew John Hughes" <ahughes at redhat.com>
> Cc: distro-pkg-dev at openjdk.java.net
> Sent: Tuesday, September 28, 2010 1:46:13 PM GMT -05:00 US/Canada Eastern
> Subject: Re: [RFC] [Plugin] Fixes 100% cpu load
>
> ----- "Dr Andrew John Hughes" <ahughes at redhat.com> wrote:
>
> > From: "Dr Andrew John Hughes" <ahughes at redhat.com>
> > To: "Andrew Su" <asu at redhat.com>
> > Cc: distro-pkg-dev at openjdk.java.net
> > Sent: Tuesday, September 28, 2010 1:17:04 PM GMT -05:00 US/Canada
> Eastern
> > Subject: Re: [RFC] [Plugin] Fixes 100% cpu load
> >
> > On 15:25 Mon 27 Sep , Andrew Su wrote:
> > > Hello,
> > >
> > > This patch fixes the following:
> > > *Plugin goes into a deadlock waiting for another thread to
> > initialize applet, and no more threads are available to consume
> > messages.
> > > *Plugin handles "destroy" message when another thread trying to
> > create the applet is kicked off processor before setting the status
> to
> > PRE_INIT.
> > >
> > >
> > > Example.
> > > We have 2 MessageConsumer threads.
> > > Passes "destroy" message to both of them for applet 1 and applet
> 2.
> > > Thread 1: Spinlock waiting for another thread to initialize
> applet
> > 1.
> > > Thread 2: Spinlock waiting for another thread to initialize
> applet
> > 2.
> > > Both threads are now in deadlock.
> > >
> > > Proposed Patch:
> > > When message is destroy, don't spinlock, handle it right away.
> > > -Problem with this: handles destroy right before we start
> > initializing the applet.
> > > -Fix to new problem: Synchronize the two blocks
> > > --Case 1:If in process of initializing, finish initializing.
> handle
> > the destroy.
> > > --Case 2:If destroy already handled, don't initialize.
> > >
> > >
> > > Cheers,
> > > Andrew
> >
> > Some points:
> >
> > * Why is the access to appLock protected by mainLock in the first
> > block
> > but the later get() is not?
> I've put that under mainLock because we don't want two thread or more
> inside the if statements and setting one of them setting it to one
> value then later after locking it, change the reference inside
> appLock.
>
> line 1 //synchronized(mainLock){
> line 2 if (!appLock.containsKey(identifier)){
> line 3 boolean[] lock = {false};
> line 4 appLock.put(identifier, lock);
> line 5 }
> line 6 //}
>
> Without locking it here with mainLock we come to the following issue.
> -Thread 1: kicked off processor at line 3
> -Thread 2: processes the if block, then continues to the later
> synchronized block (lock acquired)
> -Thread 1: resumes and reassign a new lock for identifier in appLock.
>
> Now thread 1 and thread 2 are holding two different locks.
>
> Locking the above would make sure that appLock will not get reassigned
> a different value later for the same identifier.
>
> > * On the same subject, why is appLock a Hashtable rather than a
> > HashMap?
> It could be changed to a HashMap, I'm sure it can even be implemented
> as array too.
>
>
> > Do you need the synchronisation this provides? If so, would it not
> be better
> > to synchronise a HashMap or use one of the concurrent collections
> with better
> > performance?
> I'm not quite sure I'm interpreting this question correctly, but the
> synchronization is done on the boolean[], which is much faster than
> doing it on something that would need to call a constructor.
>
> > It's unclear whether the Hashtable's internal lock is protecting its
> contents,
> > whether mainLock is or some strange combination of both.
> Hashtable does synchronization when adding and reading.
>
> > * The second synchronised block is far too big. Is this really
> > necessary?
> I would believe so, this goes back to the problem with destroying the
> applet while one thread is already in process of initializing it.
>
> > * Seems to be using 8 space indentation, making this hard to read.
> Will fix.
>
> >
> > It would help a lot if you could explain how this solution is
> supposed
> > to work,
> > and how you intend appLock to function. To that end, this should
> be
> > documented in
> > the code too.
> Will fix.
>
>
> >
> > > diff -r 5c0d756b4bb6
> > plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> > > --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> Fri
> > Sep 24 15:25:51 2010 +0100
> > > +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> Mon
> > Sep 27 14:59:43 2010 -0400
> > > @@ -360,6 +360,9 @@
> > >
> > > private static Long requestIdentityCounter = 0L;
> > >
> > > + private static Hashtable<Integer,boolean[]> appLock = new
> > Hashtable<Integer, boolean[]>();
> > > + private static int[] mainLock = {0};
> > > +
> > > /**
> > > * Null constructor to allow instantiation via
> newInstance()
> > > */
> > > @@ -505,78 +508,92 @@
> > > */
> > > public static void handleMessage(int identifier, int
> > reference, String message)
> > > {
> > > -
> > > + synchronized(mainLock){
> > > + if (!appLock.containsKey(identifier)){
> > > + boolean[] lock = {false};
> > > + appLock.put(identifier, lock);
> > > + }
> > > + }
> > > +
> > > PluginDebug.debug("PAV handling: " + message);
> > >
> > > try {
> > > if (message.startsWith("handle")) {
> > > -
> > > - // Extract the information from the message
> > > - String[] msgParts = new String[4];
> > > - for (int i=0; i < 3; i++) {
> > > - int spaceLocation = message.indexOf(' ');
> > > - int nextSpaceLocation = message.indexOf(' ',
> > spaceLocation+1);
> > > - msgParts[i] = message.substring(spaceLocation +
> 1,
> > nextSpaceLocation);
> > > - message = message.substring(nextSpaceLocation +
> 1);
> > > + synchronized(appLock.get(identifier)){
> > > + if (!status.containsKey(identifier)){
> > > + // Extract the information from the message
> > > + String[] msgParts = new String[4];
> > > + for (int i=0; i < 3; i++) {
> > > + int spaceLocation = message.indexOf(' ');
> > > + int nextSpaceLocation = message.indexOf(' ',
> > spaceLocation+1);
> > > + msgParts[i] = message.substring(spaceLocation +
> 1,
> > nextSpaceLocation);
> > > + message = message.substring(nextSpaceLocation +
> > 1);
> > > + }
> > > +
> > > + long handle = Long.parseLong(msgParts[0]);
> > > + String width = msgParts[1];
> > > + String height = msgParts[2];
> > > +
> > > + int spaceLocation = message.indexOf(' ',
> > "tag".length()+1);
> > > + String documentBase =
> > > +
> > UrlUtil.decode(message.substring("tag".length() + 1,
> spaceLocation));
> > > + String tag = message.substring(spaceLocation+1);
>
> > > +
> > > + PluginDebug.debug ("Handle = " + handle + "\n" +
> > > + "Width = " + width + "\n" +
> > > + "Height = " + height + "\n" +
> > > + "DocumentBase = " +
> documentBase
> > + "\n" +
> > > + "Tag = " + tag);
> > > +
> > > + status.put(identifier,
> > PAV_INIT_STATUS.PRE_INIT);
> > > + PluginAppletViewer.parse
> > > + (identifier, handle, width, height,
> > > + new StringReader(tag),
> > > + new URL(documentBase));
> > > +
> > > +
> > > + int maxWait = APPLET_TIMEOUT; // wait for
> applet
> > to fully load
> > > + int wait = 0;
> > > + while
> > (!status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE) &&
> > > + (wait < maxWait)) {
> > > +
> > > + try {
> > > + Thread.sleep(50);
> > > + wait += 50;
> > > + } catch (InterruptedException ie) {
> > > + // just wait
> > > + }
> > > + }
> > > +
> > > + if
> > (!status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE))
> > > + throw new Exception("Applet
> initialization
> > timeout");
> > > +
> > > + PluginAppletViewer oldFrame =
> > applets.get(identifier);
> > > + reFrame(oldFrame, oldFrame.identifier,
> > oldFrame.statusMsgStream,
> > > + handle, oldFrame.panel);
> > > + }
> > > + appLock.get(identifier).notifyAll();
> > > }
> > > -
> > > - long handle = Long.parseLong(msgParts[0]);
> > > - String width = msgParts[1];
> > > - String height = msgParts[2];
> > > -
> > > - int spaceLocation = message.indexOf(' ',
> > "tag".length()+1);
> > > - String documentBase =
> > > -
> > UrlUtil.decode(message.substring("tag".length() + 1,
> spaceLocation));
> > > - String tag = message.substring(spaceLocation+1);
> > > -
> > > - PluginDebug.debug ("Handle = " + handle + "\n" +
> > > - "Width = " + width + "\n" +
> > > - "Height = " + height + "\n" +
> > > - "DocumentBase = " + documentBase
> +
> > "\n" +
> > > - "Tag = " + tag);
> > > -
> > > - status.put(identifier,
> > PAV_INIT_STATUS.PRE_INIT);
> > > - PluginAppletViewer.parse
> > > - (identifier, handle, width, height,
> > > - new StringReader(tag),
> > > - new URL(documentBase));
> > > -
> > > -
> > > - int maxWait = APPLET_TIMEOUT; // wait for
> applet
> > to fully load
> > > - int wait = 0;
> > > - while
> > (!status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE) &&
> > > - (wait < maxWait)) {
> > > -
> > > - try {
> > > - Thread.sleep(50);
> > > - wait += 50;
> > > - } catch (InterruptedException ie) {
> > > - // just wait
> > > - }
> > > - }
> > > -
> > > - if
> > (!status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE))
> > > - throw new Exception("Applet initialization
> > timeout");
> > > -
> > > - PluginAppletViewer oldFrame =
> > applets.get(identifier);
> > > - reFrame(oldFrame, oldFrame.identifier,
> > oldFrame.statusMsgStream,
> > > - handle, oldFrame.panel);
> > > -
> > > } else {
> > > - PluginDebug.debug ("Handling message: " +
> message
> > + " instance " + identifier + " " + Thread.currentThread());
> > > -
> > > - // Wait till initialization finishes
> > > - while (!applets.containsKey(identifier) &&
> > > - (
> > > - !status.containsKey(identifier) ||
> > > -
> > status.get(identifier).equals(PAV_INIT_STATUS.PRE_INIT)
> > > - )
> > > - );
> > > -
> > > - // don't bother processing further for inactive
> > applets
> > > - if
> > (status.get(identifier).equals(PAV_INIT_STATUS.INACTIVE))
> > > - return;
> > > -
> > > + synchronized(appLock.get(identifier)){
> > > + PluginDebug.debug ("Handling message: " +
> message
> > + " instance " + identifier + " " + Thread.currentThread());
> > > +
> > > + // Wait till initialization finishes
> > > + while (!applets.containsKey(identifier) &&
> > > + (
> > > + !status.containsKey(identifier) ||
> > > +
> > status.get(identifier).equals(PAV_INIT_STATUS.PRE_INIT)
> > > + ) && !message.startsWith("destroy")
> > > + ){
> > appLock.get(identifier).wait(5*1000000000);}
> > > +
> > > + if (message.startsWith("destroy"))
> > > +
> applets.get(identifier).handleMessage(reference,
> > message);
> > > +
> > > + // don't bother processing further for
> inactive
> > applets
> > > + if
> > (status.get(identifier).equals(PAV_INIT_STATUS.INACTIVE))
> > > + return;
> > > + }
> > > +
> > >
> applets.get(identifier).handleMessage(reference,
> > message);
> > > }
> > > } catch (Exception e) {
> >
> >
> > --
> > 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
>
>
> Thanks for your comments, I hoped I cleared up some of the questions
> you had.
> Any other comments or question is welcome.
>
> Cheers,
> Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20100928_destroy_deadlock.patch
Type: text/x-patch
Size: 8863 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20100928/2ee76036/20100928_destroy_deadlock.patch
More information about the distro-pkg-dev
mailing list