[RFC] [Plugin] Fixes 100% cpu load
Andrew Su
asu at redhat.com
Tue Sep 28 14:14:02 PDT 2010
Update:
-Changed to use ConcurrentHashMap.
-Used suggestion of putIfAbsent instead of locking.
Cheers,
--Andrew
Attached:
Modified version of file.
Patch file.
----- "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 4:27:46 PM GMT -05:00 US/Canada Eastern
> Subject: Re: [RFC] [Plugin] Fixes 100% cpu load
>
> On 13:46 Tue 28 Sep , Andrew Su wrote:
> >
> > ----- "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.
> >
>
> Yeah I get that. It's taken me a bit to get my head round it, but I
> think I understand what
> you're trying to do now and I think it could be done in a simpler
> way.
>
> On first reading, I didn't realise that you were also creating
> mainLock. As it is, it seems
> the logic is:
>
> 1. Maintain a map containing locks for each identifier.
> 2. If an identifier doesn't have a lock, create one. mainLock is
> used to make this concurrent.
> 3. Once we've established we have a lock, only process a message with
> an identifier if a message is
> not being processed with the same identifier. Otherwise, block.
>
> You can make this simpler by getting rid of mainLock and using a
> ConcurrentHashMap:
> Use:
>
> private static ConcurrentMap<Integer,Object> appLock = new
> ConcurrentHashMap<Integer,Object>();
>
> in place of the current appLock and mainLock initialisation.
> Use:
>
> appLock.putIfAbsent(identifier, new Object());
>
> in place of:
>
> > 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 //}
>
> putIfAbsent is the equivalent of doing:
>
> if (!map.containsKey(key))
> return map.put(key, value);
> else
> return map.get(key);
>
> atomically (we ignore the return value).
>
> > > * 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.
>
> What I meant is that Hashtable has its own synchronisation i.e. every
> call is:
>
> lock();
> doWork();
> unlock();
>
> where the lock is on the hashtable. The same is true of other 1.0
> collection classes like Vector.
> If you don't need concurrency, this is inefficent. If you do, it's
> usually better to use one of the concurrency collections
> added in 1.5 which have better performance. The same is true of
> StringBuffer/Builder, with the former being synchronised.
>
> Using a boolean[] doesn't avoid construction, as you're creating and
> populating an array object. If you weren't creating
> an object, you wouldn't have a monitor to synchronise on. In the
> above, I've reverted to use Object as the lock as it's a
> lot clearer what's going on; I was assuming you were storing content
> in these arrays at first rather than just using them
> as locks.
>
> >
> > > 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.
> >
>
> Looking at the rest of the file, it seems you're not the initial
> culprit :-)
>
> Could you attach a copy of the changed file? It would be easier to
> examine than trying to read this diff,
> as essentially the whole method seems to be rewritten.
>
> > >
> > > 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.
> >
>
> What's still unclear to me is how these identifiers work and why this
> problem is solved by having one thread per identifier.
>
> >
> > >
> > > > 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
> >
>
> --
> 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: PluginAppletViewer.java
Type: text/x-java
Size: 80196 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20100928/5bdb0d9b/PluginAppletViewer.java
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 20100928_destroy_deadlock.patch
Type: text/x-patch
Size: 8921 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20100928/5bdb0d9b/20100928_destroy_deadlock.patch
More information about the distro-pkg-dev
mailing list