[RFC] [Plugin] Fixes 100% cpu load
Andrew Su
asu at redhat.com
Tue Sep 28 10:46:13 PDT 2010
----- "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
More information about the distro-pkg-dev
mailing list