[RFC] [Plugin] Fixes 100% cpu load
Deepak Bhole
dbhole at redhat.com
Thu Sep 30 15:53:55 PDT 2010
* Andrew Su <asu at redhat.com> [2010-09-30 17:57]:
>
> ----- "Deepak Bhole" <dbhole at redhat.com> wrote:
>
> > From: "Deepak Bhole" <dbhole at redhat.com>
> > To: "Andrew Su" <asu at redhat.com>
> > Cc: distro-pkg-dev at openjdk.java.net
> > Sent: Thursday, September 30, 2010 5:01:44 PM GMT -05:00 US/Canada Eastern
> > Subject: Re: [RFC] [Plugin] Fixes 100% cpu load
> >
> > Hi,
> >
> > Sorry I couldn't get to this earlier.. wanted to study the patch
> > carefully before commenting.
> >
> > * Andrew Su <asu at redhat.com> [2010-09-27 15:25]:
> > > 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.
> > >
> >
> > I am a bit confused about this part. As it stands now, the sequence
> > is
> > as follows:
> >
> > init message comes in -> start init
> > destroy message comes in
> > if init is done -> destroy
> > if init is in progress -> wait, then destroy
> >
> > Determination of if init is done or in progress is done via the
> > status
> > hashmap which contains one of PRE_INIT, IN_INIT or INIT_COMPLETE.
> > Destroy cannot be handled before PRE_INIT is set, as PRE_INIT is the
> > first thing set, and this code will prevent destroy from proceeding
> > until it is:
> >
> > // Wait till initialization finishes
> > while (!applets.containsKey(identifier) &&
> > (
> > !status.containsKey(identifier) ||
> >
> > status.get(identifier).equals(PAV_INIT_STATUS.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.
> > >
> >
> > I don't understand the destroy-before-init case. Are you seeing an
> > instance
> > where destroy gets sent before init does? And if that's what happens,
> > the while
> > loop above would still keep destroy from proceeding until it is
> > initialized.
> >
> > Is there a test case for this problem?
>
> You are indeed correct that that is the sequence of handling the messages, but..
> Let's say I load a page with 10 applets (we have 5 threads that handle message).
> Now before the applets load, navigate to another page that loads applets or not, then we will get destroy messages. Since the messages are queued, when there is no workers free to do work, the messages are sent to the back of the queue to be handled later. Assuming a better case where each of our 5 threads are given the handle message (this is the message we use to initialize our applets). Since now all our workers are busy processing the init, the next 5 handle messages gets dequeued then placed at the back of queue. Next the 5 threads finishes initializing the applets.
Where did you see the 5 thread count? There should be 6. 4 non-priority,
2 priority. Priority threads do not handle blocking calls, and can only
be used for messages registered as priority.
> Each dequeuing the from the readQueue. If the destroy messages are the ones initialized, works as intended, if not, all 5 threads will now go into a never-ending loop. If the user waits until all the applets load before navigating somewhere else, it'll work out since the inits will be processed and destroy will be carried out properly, otherwise it'll just take up the cpu.
>
Okay, so I have 4 free workers (let's ignore priority for now).
Thread 0, Thread 1, Thread 2 and Thread 3.
10 init messages come in. And we have:
Thread 0 - Processing init 0
Thread 1 - Processing init 1
Thread 2 - Processing init 2
Thread 3 - Processing init 3
The remaining init messages get queued. So the queue now has:
[init 3, init 4, init 5, init 6, init 7, init 8, init 9]
10 destroy messages come in. There are no free workers, so they get
queued and the readQueue is:
[init 3, init 4, init 5, init 6, init 7, init 8, init 9, destroy 0,
destroy 1, destroy 2, destroy 3, destroy 4, destroy 5, destroy 6,
destroy 7, destroy 8, destroy 9]
At some point, as the system tests each message and requeues them, the
readqueue becomes:
[destroy 4, destroy 5, destroy 6, destroy 7, destroy 8, destroy 9 ....]
And those first 4 destroy messages take up the 4 available threads.
Is this the correct description of the issue?
Assuming so, wouldn't just keeping messages at the front of the queue
instead of re-adding them to the back work? The fix would be a one-liner
and we wouldn't need any locks.
I don't think it will make things worse either. Messages are re-added to
the queue only when no free workers are available. Adding to the back
of the queue does not guarantee FIFO processing order since the order is
constantly changing (as seen in your example). However at the end of
each cycle, the order resembles FIFO. This means that FIFO order works
as well. If FIFO works, we can just make that the default.
This does no solve the problem of the applets 'hanging around in a
separate window' until destroy is processed, but I don't imagine too
many regular users see that case. And even if they do, the applet is
always guaranteed to be destroyed eventually, ensuring a consistent
state.
Cheers,
Deepak
> >
> > Sorry to be nitpicking.. I am trying to get rid of locks from the code
> > as much
> > as possible by re-designing message sequencing... if a lock is to be
> > added, I
> > want to make sure there is no other solution first :)
> >
> > Cheers,
> > Deepak
> >
> > > Cheers,
> > > Andrew
> >
> > > 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 spaceLocati
on = 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
More information about the distro-pkg-dev
mailing list