[icedtea-web] RFC: Patch to stabilize applet initialization

[Andrew Su]

----- "Deepak Bhole" <dbhole at redhat.com> wrote:

> From: "Deepak Bhole" <dbhole at redhat.com>
> To: "Andrew Su" <asu at redhat.com>
> Cc: "IcedTea Distro List" <distro-pkg-dev at openjdk.java.net>
> Sent: Friday, October 22, 2010 12:06:24 PM GMT -05:00 US/Canada Eastern
> Subject: Re: [icedtea-web] RFC: Patch to stabilize applet initialization
>
> * Andrew Su <asu at redhat.com> [2010-10-22 10:35]:
> > 
> > ----- "Deepak Bhole" <dbhole at redhat.com> wrote:
> > 
> > > From: "Deepak Bhole" <dbhole at redhat.com>
> > > To: "IcedTea Distro List" <distro-pkg-dev at openjdk.java.net>
> > > Sent: Thursday, October 21, 2010 5:41:11 PM GMT -05:00 US/Canada
> Eastern
> > > Subject: Re: [icedtea-web] RFC: Patch to stabilize applet
> initialization
> > >
> > > * Deepak Bhole <dbhole at redhat.com> [2010-10-21 17:38]:
> > > > Hi,
> > > > 
> > > > The attached patch envelopes fixes for two of Andrew's pending
> > > patches
> > > > (fixing 100% CPU load and the frame pop-up issue). In addition
> to
> > > those,
> > > > it fixes a lot of other initialization issues, particularly
> when
> > > > initializing multiple applets and/or closing other tabs with
> > > applets
> > > > which are being initialized.
> > > > 
> > > > With this patch, I tested over 200 parallel applet inits,
> randomly
> > > > closing tabs, and everything works correctly.
> > > > 
> > > > One known issue is that if timed correctly, a frame can appear
> > > outside
> > > > for a fraction of a second. It always goes away instantly
> though,
> > > and I
> > > > will be posting a patch to fix that problem in the near future.
> > > > 
> > > > Cheers,
> > > > Deepak 
> > > 
> > > Please ignore the spacing issues. I will be normalizing all
> spaces
> > > for
> > > all of icedtea-web in another commit after all pending patches
> are
> > > in.
> > > 
> > > Cheers,
> > > Deepak
> > > 
> 
> <snip>, <cleaned up spacing for the code block below>
> 
> >>private static synchronized PAV_INIT_STATUS updateStatus(int
> identifier, PAV_INIT_STATUS newStatus) {
> >>   
> >>    PAV_INIT_STATUS prev = status.get(identifier);
> >>
> >>    // If the status is set
> >>    if (status.containsKey(identifier)) {
> >>
> >>        // Nothing may override destroyed status
> >>        if
> (status.get(identifier).equals(PAV_INIT_STATUS.DESTROYED)) {
> >>            return prev;
> >>        }
> >>        
> >>        // If status is inactive, only DESTROYED may override it
> >>        if (status.get(identifier).equals(PAV_INIT_STATUS.INACTIVE))
> {
> >>            if (!newStatus.equals(PAV_INIT_STATUS.DESTROYED)) {
> >>                return prev;
> >>            }
> >>        }
> >>    }
> > 
> > Status isn't updated to PAV_INIT_STATUS.DESTROYED.
> 
> Hmm, what do you mean? It is in the line below..
I realized, misread the line. :)

> 
> >>
> >>    // Else set to given status
> >>    status.put(identifier, newStatus);
> >>        
> >>    return prev;
> >>}
> <snip>
> 
> > 
> > I believe that splitting it to atleast two queue, one for priority
> and one for all other messages would be a bit better performance wise.
> 
> > Since:
> > -- getPriorityStrIfPriority(): assume constant
> > -- Adding to proper queue: constant
> > -- Checking if queue is empty: constant
> > -- Reading and deleting message: constant. 
> > Right now, doing N checks for each time we try to read a message
> would be a bit more expensive. 
> > 
> > Splitting it up could potentially save us one extra call to
> getPriorityStrIfPriority() for each message. 
> > Using an extra queue, let's call it PQ, we can store the priorityStr
> into PQ if we determined it is a priority message, this way we don't
> need to call getPriorityStrIfPriority() on each message again later. 
> > 
> 
> I understand and agree with your concern of scanning each message
> multiple times, I don't like it either. I don't think multiple queues
> is
> the way to go though. IMO a better solution is to have readQueue store
> a
> structure that stores if a message is priority or not.
> 
> Let's say 5 messages come in. They require 5 scans with the 2 queue
> model to do the placement.
> 
> With a structure-in-queue model, we have:
> 
> Process each message as non-priority. First 2 messages go away. Then
> we
> run bringPriorityMessagesToFront, which scans the remaining 3, and
> the
> priority handlers take care of them.
> 
> The above model becomes inefficient if the queue size is too large.
> However that is extremely unlikely given that the queue only fills up
> during multiple-applet init, which is rare. During an average run,
> queue
> size is 0-2.
> 
> What do you think of the above approach?
> 
The above method still requires going through the list of messages. As a compromise, I propose that...
We keep a structure in readQueue to keep track if it is priority or not.
With that implemented we can simply do the check when queuing: Place message to front if priority and append otherwise.

This would remove the need to scan the list of messages and removes the need for multiple queue. It'll be sorted already
As for the structure used in the queue, it could be simply storing the priorityStr, or null.

It's true on average we won't be getting an overwhelming amount of messages, but just to cover all our bases :)

So, how about this idea?

Cheers,
 Andrew


> Cheers,
> Deepak
> 
> > 
> > > > +
> > > >  	    public void run() {
> > > >  
> > > >  	        while (true) {
> > > > @@ -190,7 +219,6 @@
> > > >  
> > > >  	                String[] msgParts = message.split(" ");
> > > >  
> > > > -
> > > >  	                String priorityStr =
> > > getPriorityStrIfPriority(message);
> > > >  	                boolean isPriorityResponse = (priorityStr !=
> > > null);
> > > >  		
> > > > @@ -199,9 +227,12 @@
> > > >  	                
> > > >  	                if (worker == null) {
> > > >  	                    synchronized(readQueue) {
> > > > -                            readQueue.addLast(message);
> > > > +                            readQueue.addFirst(message);
> > > >                          }
> > > >  
> > > > +	                    // re-scan to see if any priority message
> came
> > > in
> > > > +                        bringPriorityMessagesToFront();
> > > > +	                    
> > > >  	                    continue; // re-loop to try next msg
> > > >  	                }
> > > >
> > 
> > Just the one change to updateStatus.
> > The extra queue idea gives a minor boost for performance. (This
> change is optional, and can go in later if needed)
> > Everything else looks fine. Okay to commit after the change.
> > 
> > Cheers,
> >  Andrew