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

[Deepak Bhole]

* 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..

>>
>>    // 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?

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