[icedtea-web] RFC: Patch to stabilize applet initialization
Andrew Su
asu at redhat.com
Fri Oct 22 08:40:32 PDT 2010
----- "Andrew Su" <asu at redhat.com> wrote:
> From: "Andrew Su" <asu at redhat.com>
> To: "Deepak Bhole" <dbhole at redhat.com>
> Cc: "IcedTea Distro List" <distro-pkg-dev at openjdk.java.net>
> Sent: Friday, October 22, 2010 10:35:01 AM GMT -05:00 US/Canada Eastern
> Subject: Re: [icedtea-web] RFC: Patch to stabilize applet initialization
>
> ----- "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
> >
> > > diff -r 85db7b3a1c93
> > plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> > > --- a/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> Thu
> > Oct 21 21:12:21 2010 +0100
> > > +++ b/plugin/icedteanp/java/sun/applet/PluginAppletViewer.java
> Thu
> > Oct 21 17:22:56 2010 -0700
> > > @@ -136,12 +136,10 @@
> > > }
> > > }
> > > });
> > > +
> > > + // create the frame.
> > > + PluginAppletViewer.reFrame(null, identifier,
> System.out,
> > 0, panel);
> > >
> > > -
> > > -
> > > - // create the frame.
> > > - PluginAppletViewer.reFrame(null, identifier,
> System.out,
> > handle, panel);
> > > -
> > > panel.init();
> > >
> > > // Start the applet
> > > @@ -179,15 +177,7 @@
> > >
> > > // Wait for the panel to initialize
> > > // (happens in a separate thread)
> > > - while (panel.getApplet() == null &&
> > > - ((NetxPanel) panel).isAlive()) {
> > > - try {
> > > - Thread.sleep(50);
> > > - PluginDebug.debug("Waiting for applet to
> > initialize...");
> > > - } catch (InterruptedException ie) {
> > > - // just wait
> > > - }
> > > - }
> > > + PluginAppletViewer.waitForAppletInit((NetxPanel)
> panel);
> > >
> > > a = panel.getApplet();
> > >
> > > @@ -317,7 +307,17 @@
> > > */
> > > private static String defaultSaveFile = "Applet.ser";
> > >
> > > - private static enum PAV_INIT_STATUS {PRE_INIT, IN_INIT,
> > INIT_COMPLETE, INACTIVE};
> > > +
> > > + /**
> > > + * Enumerates the current status of an applet
> > > + *
> > > + * PRE_INIT -> Parsing and initialization phase
> > > + * INIT_COMPLETE -> Initialization complete, reframe
> pending
> > > + * REFRAME_COMPLETE -> Reframe complete, applet is
> > initialized and usable by the user
> > > + * INACTIVE -> Browser has directed that the applet be
> > destroyed (this state is non-overridable except by DESTROYED)
> > > + * DESTROYED -> Applet has been destroyed
> > > + */
> > > + private static enum PAV_INIT_STATUS {PRE_INIT,
> INIT_COMPLETE,
> > REFRAME_COMPLETE, INACTIVE, DESTROYED};
> > >
> > > /**
> > > * The panel in which the applet is being displayed.
> > > @@ -350,7 +350,6 @@
> > >
> > > private static HashMap<Integer, PAV_INIT_STATUS> status =
> > > new HashMap<Integer,PAV_INIT_STATUS>();
> > > -
> > >
> > > private long handle = 0;
> > > private WindowListener windowEventListener = null;
> > > @@ -380,16 +379,20 @@
> > > return;
> > >
> > > PluginAppletViewer newFrame = new
> > PluginAppletViewer(handle, identifier, statusMsgStream, panel);
> > > -
> > > +
> > > if (oldFrame != null) {
> > > applets.remove(oldFrame.identifier);
> > >
> > oldFrame.removeWindowListener(oldFrame.windowEventListener);
> > >
> > panel.removeAppletListener(oldFrame.appletEventListener);
> > > +
> > > + // Add first, remove later
> > > + newFrame.add("Center", panel);
> > > oldFrame.remove(panel);
> > > oldFrame.dispose();
> > > + } else {
> > > + newFrame.add("Center", panel);
> > > }
> > >
> > > - newFrame.add("Center", panel);
> > > newFrame.pack();
> > >
> > > newFrame.appletEventListener = new
> > AppletEventListener(newFrame, newFrame);
> > > @@ -397,11 +400,6 @@
> > >
> > > applets.put(identifier, newFrame);
> > >
> > > - // dispose oldframe if necessary
> > > - if (oldFrame != null) {
> > > - oldFrame.dispose();
> > > - }
> > > -
> > > PluginDebug.debug(panel + " reframed");
> > > }
> > >
> > > @@ -449,7 +447,7 @@
> > > this.frame = frame;
> > > this.appletViewer = appletViewer;
> > > }
> > > -
> > > +
> > > public void appletStateChanged(AppletEvent evt)
> > > {
> > > AppletPanel src = (AppletPanel)evt.getSource();
> > > @@ -483,9 +481,9 @@
> > > AppletPanel.changeFrameAppContext(frame,
> > SunToolkit.targetToAppContext(a));
> > > else
> > > AppletPanel.changeFrameAppContext(frame,
> > AppContext.getAppContext());
> > > -
> > > - status.put(appletViewer.identifier,
> > PAV_INIT_STATUS.INIT_COMPLETE);
> > > -
> > > +
> > > + updateStatus(appletViewer.identifier,
> > PAV_INIT_STATUS.INIT_COMPLETE);
> > > +
> > > break;
> > > }
> > > }
> > > @@ -510,7 +508,13 @@
> > >
> > > try {
> > > if (message.startsWith("handle")) {
> > > -
> > > +
> > > + // If there is a key for this status, it means it
> > > + // was either initialized before, or destroy has
> been
> >
> > > + // processed. Stop moving further.
> > > + if (updateStatus(identifier,
> > PAV_INIT_STATUS.PRE_INIT) != null)
> > > + return;
> > > +
> > > // Extract the information from the message
> > > String[] msgParts = new String[4];
> > > for (int i=0; i < 3; i++) {
> > > @@ -519,32 +523,38 @@
> > > 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);
> > > + String tag = message.substring(spaceLocation+1);
> > > +
> > > + // Decode the tag
> > > + tag = tag.replace(">", ">");
> > > + tag = tag.replace("<", "<");
> > > + tag = tag.replace("&", "&");
> > > + tag = tag.replace(" ", "\n");
> > > + tag = tag.replace(" ", "\r");
> > > + tag = tag.replace(""", "\"");
> > >
> > > 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) &&
> > > + while ( !applets.containsKey(identifier) && //
> Map
> > is populated only by reFrame
> > > (wait < maxWait)) {
> > >
> > > try {
> > > @@ -555,9 +565,44 @@
> > > }
> > > }
> > >
> > > - if
> > (!status.get(identifier).equals(PAV_INIT_STATUS.INIT_COMPLETE))
> > > + // If wait exceeded maxWait, we timed out.
> Throw
> > an exception
> > > + if (wait >= maxWait)
> > > throw new Exception("Applet initialization
> > timeout");
> > >
> > > + PluginAppletViewer oldFrame =
> > applets.get(identifier);
> > > +
> > > + // We should not try to destroy an applet during
>
> > > + // initialization. It may cause an inconsistent
> > state,
> > > + // which would bad if it's a trusted applet that
>
> > > + // read/writes to files
> > > + waitForAppletInit((NetxPanel)
> > applets.get(identifier).panel);
> > > +
> > > + // Should we proceed with reframing?
> > > + if
> > (status.get(identifier).equals(PAV_INIT_STATUS.INACTIVE)) {
> > > + destroyApplet(identifier);
> > > + return;
> > > + }
> > > +
> > > + // Proceed with re-framing
> > > + reFrame(oldFrame, identifier, System.out, handle,
> > oldFrame.panel);
> > > +
> > > + // There is a slight chance that destroy can
> > happen
> > > + // between the above and below line
> > > + if (updateStatus(identifier,
> > PAV_INIT_STATUS.REFRAME_COMPLETE).equals(PAV_INIT_STATUS.INACTIVE))
> {
> > > + destroyApplet(identifier);
> > > + }
> > > +
> > > + } else if (message.startsWith("destroy")) {
> > > +
> > > + // Set it inactive, and try to do cleanup is
> > applicable
> > > + PAV_INIT_STATUS previousStatus =
> > updateStatus(identifier, PAV_INIT_STATUS.INACTIVE);
> > > + PluginDebug.debug("Destroy status set for " +
> > identifier);
> > > +
> > > + if (previousStatus != null &&
> > > +
> > previousStatus.equals(PAV_INIT_STATUS.REFRAME_COMPLETE)) {
> > > + destroyApplet(identifier);
> > > + }
> > > +
> > > } else {
> > > PluginDebug.debug ("Handling message: " +
> message
> > + " instance " + identifier + " " + Thread.currentThread());
> > >
> > > @@ -568,7 +613,7 @@
> > >
> > 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;
> > > @@ -580,31 +625,131 @@
> > > e.printStackTrace();
> > >
> > > // If an exception happened during pre-init, we
> need
> > to update status
> > > - if
> > (status.get(identifier).equals(PAV_INIT_STATUS.PRE_INIT))
> > > - status.put(identifier,
> PAV_INIT_STATUS.INACTIVE);
> > > + updateStatus(identifier, PAV_INIT_STATUS.INACTIVE);
> > >
> > > throw new RuntimeException("Failed to handle
> message:
> > " +
> > > message + " for instance " + identifier,
> e);
> > > }
> > > }
> > > -
> > > +
> > > + /**
> > > + * Sets the status unless an overriding status is set (e.g.
> if
> >
> > > + * status is DESTROYED, it may not be overridden).
> > > + *
> > > + * @param identifier The identifier for which the status is
> to
> > be set
> > > + * @param status The status to switch to
> > > + * @return The previous status
> > > + */
> > > + 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.
Oops! Sorry misread that. This is fine, (didn't see the !)
>
>
> > > + }
> > > + }
> > > + }
> > > +
> > > + // Else set to given status
> > > + status.put(identifier, newStatus);
> > > +
> > > + return prev;
> > > + }
> > > +
> > > + /**
> > > + * Destroys the given applet instance.
> > > + *
> > > + * This function may be called multiple times without
> > problems.
> > > + * It does a synchronized check on the status and will only
>
> > > + * attempt to destroy the applet if not previously
> destroyed.
> > > + *
> > > + * @param identifier The instance which is to be destroyed
> > > + */
> > > +
> > > + private static synchronized void destroyApplet(int
> identifier)
> > {
> > > +
> > > + PluginDebug.debug("DestroyApplet called for " +
> identifier);
> > > +
> > > + PAV_INIT_STATUS prev = updateStatus(identifier,
> > PAV_INIT_STATUS.DESTROYED);
> > > +
> > > + // If already destroyed, return
> > > + if (prev.equals(PAV_INIT_STATUS.DESTROYED)) {
> > > + PluginDebug.debug(identifier + " already destroyed.
> > Returning.");
> > > + return;
> > > + }
> > > +
> > > + // If already disposed, return
> > > + if (applets.get(identifier).panel.applet == null) {
> > > + // Try to still dispose the panel itself -- no harm done
> > with double dispose
> > > + applets.get(identifier).dispose();
> > > +
> > > + PluginDebug.debug(identifier + " inactive. Returning.");
> > > + return;
> > > + }
> > > +
> > > + PluginDebug.debug("Attempting to destroy " + identifier);
> > > +
> > > + final int fIdentifier = identifier;
> > > + SwingUtilities.invokeLater(new Runnable() {
> > > + public void run() {
> > > + applets.get(fIdentifier).appletClose();
> > > + }
> > > + });
> > > +
> > > + PluginDebug.debug(identifier + " destroyed");
> > > + }
> > > +
> > > + /**
> > > + * Function to block until applet initialization is
> complete
> > > + *
> > > + * @param identifier The instance to wait for
> > > + */
> > > + public static void waitForAppletInit(NetxPanel panel) {
> > > +
> > > + int waitTime = 0;
> > > +
> > > + // Wait till initialization finishes
> > > + while (panel.getApplet() == null &&
> > > + panel.isAlive() &&
> > > + waitTime < APPLET_TIMEOUT) {
> > > + try {
> > > + if (waitTime%500 == 0)
> > > + PluginDebug.debug("Waiting for applet panel "
> +
> > panel + " to initialize...");
> > > +
> > > + Thread.sleep(waitTime += 50);
> > > + } catch (InterruptedException ie) {
> > > + // just wait
> > > + }
> > > + }
> > > +
> > > + PluginDebug.debug("Applet panel " + panel + "
> > initialized");
> > > + }
> > > +
> > > public void handleMessage(int reference, String message)
> > > {
> > > if (message.startsWith("width")) {
> > >
> > > // Wait for panel to come alive
> > > int maxWait = APPLET_TIMEOUT; // wait for panel to
> > come alive
> > > - int wait = 0;
> > > + 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
> > > - }
> > > - }
> > > -
> > > + try {
> > > + Thread.sleep(50);
> > > + wait += 50;
> > > + } catch (InterruptedException ie) {
> > > + // just wait
> > > + }
> > > + }
> > >
> > > // 0 => width, 1=> width_value, 2 => height, 3=>
> > height_value
> > > String[] dimMsg = message.split(" ");
> > > @@ -636,7 +781,7 @@
> > >
> > > panel.setSize(width, height);
> > > panel.validate();
> > > -
> > > +
> > > panel.applet.resize(width, height);
> > > panel.applet.validate();
> > > }
> > > @@ -649,9 +794,6 @@
> > > e.printStackTrace();
> > > }
> > >
> > > - } else if (message.startsWith("destroy")) {
> > > - dispose();
> > > - status.put(identifier, PAV_INIT_STATUS.INACTIVE);
> > > } else if (message.startsWith("GetJavaObject")) {
> > >
> > > // FIXME: how do we determine what security context
> > this
> > > @@ -672,15 +814,7 @@
> > >
> > > // Wait for the panel to initialize
> > > // (happens in a separate thread)
> > > - while (panel.getApplet() == null &&
> > > - ((NetxPanel) panel).isAlive()) {
> > > - try {
> > > - Thread.sleep(50);
> > > - PluginDebug.debug("Waiting for applet to
> > initialize...");
> > > - } catch (InterruptedException ie) {
> > > - // just wait
> > > - }
> > > - }
> > > + waitForAppletInit((NetxPanel) panel);
> > >
> > > PluginDebug.debug(panel + " -- " + panel.getApplet()
> +
> > " -- " + ((NetxPanel) panel).isAlive());
> > >
> > > @@ -1548,10 +1682,11 @@
> > > if (countApplets() == 0) {
> > > appletSystemExit();
> > > }
> > > +
> > > + updateStatus(identifier,
> > PAV_INIT_STATUS.DESTROYED);
> > > }
> > > }).start();
> > >
> > > - status.put(identifier, PAV_INIT_STATUS.INACTIVE);
> > > }
> > >
> > > /**
> > > @@ -1677,18 +1812,6 @@
> > > val = buf.toString();
> > > }
> > >
> > > - att = att.replace(">", ">");
> > > - att = att.replace("<", "<");
> > > - att = att.replace("&", "&");
> > > - att = att.replace(" ", "\n");
> > > - att = att.replace(" ", "\r");
> > > -
> > > - val = val.replace(">", ">");
> > > - val = val.replace("<", "<");
> > > - val = val.replace("&", "&");
> > > - val = val.replace(" ", "\n");
> > > - val = val.replace(" ", "\r");
> > > -
> > > PluginDebug.debug("PUT " + att + " = '" + val + "'");
> > > atts.put(att.toLowerCase(java.util.Locale.ENGLISH),
> val);
> > >
> > > @@ -1708,7 +1831,6 @@
> > > // private static final == inline
> > > private static final boolean isInt(Object o) {
> > > boolean isInt = false;
> > > -
> > > try {
> > > Integer.parseInt((String) o);
> > > isInt = true;
> > > @@ -1763,7 +1885,6 @@
> > > } catch (IOException ioe) {
> > > return ioe;
> > > }
> > > -
> > > return null;
> > > }
> > > };
> > > @@ -1785,6 +1906,7 @@
> > > boolean isObjectTag = false;
> > > boolean isEmbedTag = false;
> > > boolean objectTagAlreadyParsed = false;
> > > +
> > > // The current character
> > > // FIXME: This is an evil hack to force
> > pass-by-reference.. the
> > > // parsing code needs to be rewritten from scratch to
> > prevent such
> > > @@ -1879,19 +2001,6 @@
> > > if (val == null) {
> > >
> > statusMsgStream.println(requiresNameWarning);
> > > } else if (atts != null) {
> > > - att = att.replace(">", ">");
> > > - att = att.replace("<", "<");
> > > - att = att.replace("&",
> "&");
> > > - att = att.replace(" ",
> "\n");
> > > - att = att.replace(" ",
> "\r");
> > > - att = att.replace(""",
> > "\"");
> > > -
> > > - val = val.replace(">", ">");
> > > - val = val.replace("<", "<");
> > > - val = val.replace("&",
> "&");
> > > - val = val.replace(" ",
> "\n");
> > > - val = val.replace(" ",
> "\r");
> > > - val = val.replace(""",
> > "\"");
> > > PluginDebug.debug("PUT " + att +
> "
> > = " + val);
> > > atts.put(att.toLowerCase(),
> val);
> > > } else {
> > > @@ -1920,8 +2029,8 @@
> > >
> > > if (atts.get("width") == null ||
> > !isInt(atts.get("width"))) {
> > > atts.put("width", width);
> > > - }
> > > -
> > > + }
> > > +
> > > if (atts.get("height") == null ||
> > !isInt(atts.get("height"))) {
> > > atts.put("height", height);
> > > }
> > > @@ -1974,7 +2083,7 @@
> > > if (atts.get("width") == null ||
> > !isInt(atts.get("width"))) {
> > > atts.put("width", width);
> > > }
> > > -
> > > +
> > > if (atts.get("height") == null ||
> > !isInt(atts.get("height"))) {
> > > atts.put("height", height);
> > > }
> > > @@ -2023,10 +2132,11 @@
> > > if (atts.get("width") == null ||
> > !isInt(atts.get("width"))) {
> > > atts.put("width", width);
> > > }
> > > -
> > > +
> > > if (atts.get("height") == null ||
> > !isInt(atts.get("height"))) {
> > > atts.put("height", height);
> > > }
> > > +
> > > }
> > > }
> > > }
> > > diff -r 85db7b3a1c93
> > plugin/icedteanp/java/sun/applet/PluginMessageConsumer.java
> > > --- a/plugin/icedteanp/java/sun/applet/PluginMessageConsumer.java
> > Thu Oct 21 21:12:21 2010 +0100
> > > +++ b/plugin/icedteanp/java/sun/applet/PluginMessageConsumer.java
> > Thu Oct 21 17:22:56 2010 -0700
> > > @@ -126,6 +126,10 @@
> > > }
> > >
> > > private String getPriorityStrIfPriority(String message) {
> > > +
> > > + // Destroy messages are permanently high priority
> > > + if (message.endsWith("destroy"))
> > > + return "destroy";
> > >
> > > synchronized (priorityWaitQueue) {
> > > Iterator<String> it = priorityWaitQueue.iterator();
> > > @@ -150,7 +154,6 @@
> > > }
> > > }
> > >
> > > -
> > > public void notifyWorkerIsFree(PluginMessageHandlerWorker
> worker)
> > {
> > > synchronized (initWorkers) {
> > > Iterator<Integer> i = initWorkers.keySet().iterator();
> > > @@ -176,6 +179,32 @@
> > > }
> > >
> > > protected class ConsumerThread extends Thread {
> > > +
> > > + /**
> > > + * Scans the readQueue for priority messages and brings them
> to
> > the front
> > > + */
> > > + private void bringPriorityMessagesToFront() {
> > > + synchronized (readQueue) {
> > > +
> > > + // iterate through the list
> > > + for (int i=0; i < readQueue.size(); i++) {
> > > +
> > > + // remove element at i to inspect it
> > > + String message = readQueue.remove(i);
> > > +
> > > + // if element at i is a priority msg, bring it forward
> > > + if (getPriorityStrIfPriority(message) != null) {
> > > + readQueue.addFirst(message);
> > > + } else { // else keep it where it was
> > > + readQueue.add(i, message);
> > > + }
> > > +
> > > + // by the end the queue size is the same, so the
> > > + // position indicator (i) is still valid
> > > + }
> > > + }
> > > + }
>
> 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.
>
>
> > > +
> > > 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
More information about the distro-pkg-dev
mailing list