[RFC][Icedtea-Web]: more refactoring + dead code removal
Denis Lila
dlila at redhat.com
Mon Mar 7 08:13:30 PST 2011
> > I assume you used Eclipse to do most of this refactoring?
>
> What makes you think that?
Well, it's true :-D (I also used grep, when in doubt). Although,
the code wasn't very good about declaring private things that
should have been private, so I had to do some guesswork and make
them private and see whether I got unused warnings. One change
couldn't have been spotted by eclipse, since everything was being
used, but the container used was write only (and hence a kind of
memory leak (but not really, because the C++ side never sent
the message necessary for that code path to execute). We still
did a useless String.split though, and I highly doubt that
would have been optimized away by the compiler).
Regards,
Denis.
----- Original Message -----
> On 18:27 Fri 04 Mar , Deepak Bhole wrote:
> > * Denis Lila <dlila at redhat.com> [2011-03-02 10:51]:
> > > Hi.
> > >
> > > I've been looking at the java side of the plugin more
> > > and I think we should make the attached changes. Mostly
> > > they're removing unused code (in one case (a String.split call)
> > > the code is executed, but it's still useless).
> > >
> > > Other than that, I've made some declarations private/static/final,
> > > and in a few cases moved code around so it was easier to read
> > > (like grouping static variables together, and moving a constructor
> > > closer to the top of the class, where the variable declarations
> > > are).
> > >
> > > I also fixed a typo (I think it was a typo).
> > >
> > > I'm a bit uneasy about some of these changes. In particular,
> > > removing
> > > the SecurityManager argument to PluginMessageHandlerWorker and
> > > removing
> > > the creation of pav in PluginStreamHandler, so some feedback would
> > > be nice.
> >
>
> > In terms of
> > removed arguments like SecurityManager, they are not used (they
> > probably
> > were at some point in the past).
> >
> > Based on the glance I've taken at the change, they fine to me. Okay
> > for
> > HEAD.
> >
> > Cheers,
> > Deepak
> >
> > >
> > > Thank you,
> > > Denis.
> >
> > > diff -r 738c9c852dda ChangeLog
> > > --- a/ChangeLog Tue Mar 01 11:55:31 2011 -0500
> > > +++ b/ChangeLog Wed Mar 02 10:28:47 2011 -0500
> > > @@ -1,3 +1,29 @@
> > > +2011-03-02 Denis Lila <dlila at redhat.com>
> > > +
> > > + *
> > > plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java:
> > > + (prepopulateMethod) removed unused object o.
> > > + * plugin/icedteanp/java/sun/applet/PluginCallRequest.java:
> > > + Made all the members private.
> > > + * plugin/icedteanp/java/sun/applet/PluginMessageConsumer.java:
> > > + Removed unused imports.
> > > + (MAX_PARALLEL_INITS, MAX_WORKERS, PRIORITY_WORKERS, readQueue,
> > > + workers, streamHandler, consumerThread,
> > > + registerPriorityWait(String), unRegisterPriorityWait(String)):
> > > + made private.
> > > + (initWorkers, as, processedIds, unRegisterPriorityWait(Long),
> > > + addToInitWorkers): removed - unused.
> > > + (getPriorityStrIfPriority): made static; replaced while with
> > > for-each.
> > > + (notifyWorkerIsFree): removed synchronized section - useless.
> > > + (ConsumerThread.run): removed call to addToInitWorkers.
> > > + *
> > > plugin/icedteanp/java/sun/applet/PluginMessageHandlerWorker.java:
> > > + Removed explicit member initializations to the default values;
> > > fixed typo.
> > > + (PluginMessageHandlerWorker): Removed SecurityManager argument -
> > > unused.
> > > + * plugin/icedteanp/java/sun/applet/PluginStreamHandler.java:
> > > + Removed unused imports.
> > > + (consumer, shuttingDown): made private.
> > > + (pav, writeQueue, getMessage, messageAvailable): removed -
> > > unused.
> > > + (PluginStreamHandler): removed pav initialization.
> > > +
> > > 2011-03-01 Andrew Su <asu at redhat.com>
> > >
> > > * netx/net/sourceforge/jnlp/controlpanel/ControlPanel.java
> > > diff -r 738c9c852dda
> > > plugin/icedteanp/java/sun/applet/AppletSecurityContextManager.java
> > > ---
> > > a/plugin/icedteanp/java/sun/applet/AppletSecurityContextManager.java
> > > Tue Mar 01 11:55:31 2011 -0500
> > > +++
> > > b/plugin/icedteanp/java/sun/applet/AppletSecurityContextManager.java
> > > Wed Mar 02 10:28:47 2011 -0500
> > > @@ -43,7 +43,6 @@
> > > public class AppletSecurityContextManager {
> > >
> > > // Context identifier -> PluginAppletSecurityContext object.
> > > - // FIXME: make private
> > > private static HashMap<Integer, PluginAppletSecurityContext>
> > > contexts =
> > > new HashMap<Integer, PluginAppletSecurityContext>();
> > >
> > > diff -r 738c9c852dda
> > > plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java
> > > ---
> > > a/plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java
> > > Tue Mar 01 11:55:31 2011 -0500
> > > +++
> > > b/plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java
> > > Wed Mar 02 10:28:47 2011 -0500
> > > @@ -1228,15 +1228,14 @@
> > > Class<?> c = (Class<?>) store.getObject(classID);
> > > Method m = null;
> > > Constructor cs = null;
> > > - Object o = null;
> > >
> > > try {
> > > if (methodName.equals("<init>")
> > > || methodName.equals("<clinit>"))
> > > || {
> > > - o = cs = c.getConstructor(signature.getClassArray());
> > > + cs = c.getConstructor(signature.getClassArray());
> > > store.reference(cs);
> > > } else {
> > > - o = m = c.getMethod(methodName, signature.getClassArray());
> > > + m = c.getMethod(methodName, signature.getClassArray());
> > > store.reference(m);
> > > }
> > > } catch (NoSuchMethodException e) {
> > > diff -r 738c9c852dda
> > > plugin/icedteanp/java/sun/applet/PluginCallRequest.java
> > > --- a/plugin/icedteanp/java/sun/applet/PluginCallRequest.java Tue
> > > Mar 01 11:55:31 2011 -0500
> > > +++ b/plugin/icedteanp/java/sun/applet/PluginCallRequest.java Wed
> > > Mar 02 10:28:47 2011 -0500
> > > @@ -40,10 +40,10 @@
> > > // FIXME: for each type of request extend a new (anonymous?)
> > > // PluginCallRequest.
> > > public abstract class PluginCallRequest {
> > > - String message;
> > > - Long reference;
> > > - PluginCallRequest next;
> > > - boolean done = false;
> > > + private String message;
> > > + private Long reference;
> > > + private PluginCallRequest next;
> > > + private boolean done = false;
> > >
> > > public PluginCallRequest(String message, Long reference) {
> > > this.message = message;
> > > diff -r 738c9c852dda
> > > plugin/icedteanp/java/sun/applet/PluginMessageConsumer.java
> > > --- a/plugin/icedteanp/java/sun/applet/PluginMessageConsumer.java
> > > Tue Mar 01 11:55:31 2011 -0500
> > > +++ b/plugin/icedteanp/java/sun/applet/PluginMessageConsumer.java
> > > Wed Mar 02 10:28:47 2011 -0500
> > > @@ -38,29 +38,29 @@
> > > package sun.applet;
> > >
> > > import java.util.ArrayList;
> > > -import java.util.Hashtable;
> > > -import java.util.Iterator;
> > > import java.util.LinkedList;
> > >
> > > class PluginMessageConsumer {
> > >
> > > - private static int MAX_PARALLEL_INITS = 1;
> > > + private static final int MAX_PARALLEL_INITS = 1;
> > >
> > > // Each initialization requires 5 responses (tag, handle,
> > > width, proxy, cookie)
> > > // before the message stack unlocks/collapses. This works out
> > > well because we
> > > // want to allow upto 5 parallel tasks anyway
> > > - private static int MAX_WORKERS = MAX_PARALLEL_INITS * 4;
> > > - private static int PRIORITY_WORKERS = MAX_PARALLEL_INITS * 2;
> > > + private static final int MAX_WORKERS = MAX_PARALLEL_INITS * 4;
> > > + private static final int PRIORITY_WORKERS = MAX_PARALLEL_INITS *
> > > 2;
> > >
> > > - private static Hashtable<Integer, PluginMessageHandlerWorker>
> > > initWorkers = new Hashtable<Integer,
> > > PluginMessageHandlerWorker>(2);
> > > + private static LinkedList<String> priorityWaitQueue = new
> > > LinkedList<String>();
> > >
> > > - LinkedList<String> readQueue = new LinkedList<String>();
> > > - private static LinkedList<String> priorityWaitQueue = new
> > > LinkedList<String>();
> > > - ArrayList<PluginMessageHandlerWorker> workers = new
> > > ArrayList<PluginMessageHandlerWorker>();
> > > - PluginStreamHandler streamHandler = null;
> > > - AppletSecurity as;
> > > - ConsumerThread consumerThread = new ConsumerThread();
> > > - private static ArrayList<Integer> processedIds = new
> > > ArrayList<Integer>();
> > > + private LinkedList<String> readQueue = new LinkedList<String>();
> > > + private ArrayList<PluginMessageHandlerWorker> workers = new
> > > ArrayList<PluginMessageHandlerWorker>();
> > > + private PluginStreamHandler streamHandler;
> > > + private ConsumerThread consumerThread = new ConsumerThread();
> > > +
> > > + public PluginMessageConsumer(PluginStreamHandler streamHandler)
> > > {
> > > + this.streamHandler = streamHandler;
> > > + this.consumerThread.start();
> > > + }
> > >
> > > /**
> > > * Registers a reference to wait for. Responses to registered
> > > priority
> > > @@ -78,78 +78,45 @@
> > > *
> > > * @param searchString the string to look for in a response
> > > */
> > > - public static void registerPriorityWait(String searchString) {
> > > + private static void registerPriorityWait(String searchString) {
> > > PluginDebug.debug("Registering priority for string " +
> > > searchString);
> > > synchronized (priorityWaitQueue) {
> > > - if (!priorityWaitQueue.contains(searchString))
> > > + if (!priorityWaitQueue.contains(searchString)) {
> > > priorityWaitQueue.add(searchString);
> > > + }
> > > }
> > > }
> > >
> > > /**
> > > - * Unregisters a priority reference to wait for.
> > > - *
> > > - * @param reference The reference to remove
> > > - */
> > > - public static void unRegisterPriorityWait(Long reference) {
> > > - unRegisterPriorityWait(reference.toString());
> > > - }
> > > -
> > > - /**
> > > * Unregisters a priority string to wait for.
> > > *
> > > * @param searchString The string to unregister from the
> > > priority list
> > > */
> > > - public static void unRegisterPriorityWait(String searchString) {
> > > + private static void unRegisterPriorityWait(String searchString)
> > > {
> > > synchronized (priorityWaitQueue) {
> > > priorityWaitQueue.remove(searchString);
> > > }
> > > }
> > >
> > > - public PluginMessageConsumer(PluginStreamHandler streamHandler)
> > > {
> > > -
> > > - as = new AppletSecurity();
> > > - this.streamHandler = streamHandler;
> > > - this.consumerThread.start();
> > > - }
> > > -
> > > - private String getPriorityStrIfPriority(String message) {
> > > + private static String getPriorityStrIfPriority(String message) {
> > >
> > > // Destroy messages are permanently high priority
> > > - if (message.endsWith("destroy"))
> > > + if (message.endsWith("destroy")) {
> > > return "destroy";
> > > + }
> > >
> > > synchronized (priorityWaitQueue) {
> > > - Iterator<String> it = priorityWaitQueue.iterator();
> > > -
> > > - while (it.hasNext()) {
> > > - String priorityStr = it.next();
> > > - if (message.indexOf(priorityStr) > 0)
> > > - return priorityStr;
> > > + for (String priorityStr : priorityWaitQueue) {
> > > + if (message.indexOf(priorityStr) > 0) {
> > > + return priorityStr;
> > > + }
> > > }
> > > }
> > >
> > > return null;
> > > }
> > >
> > > - private void addToInitWorkers(Integer instanceNum,
> > > PluginMessageHandlerWorker worker) {
> > > - synchronized (initWorkers) {
> > > - initWorkers.put(instanceNum, worker);
> > > - }
> > > - }
> > > -
> > > public void notifyWorkerIsFree(PluginMessageHandlerWorker
> > > worker) {
> > > - synchronized (initWorkers) {
> > > - Iterator<Integer> i = initWorkers.keySet().iterator();
> > > - while (i.hasNext()) {
> > > - Integer key = i.next();
> > > - if (initWorkers.get(key).equals(worker)) {
> > > - processedIds.add(key);
> > > - initWorkers.remove(key);
> > > - }
> > > - }
> > > - }
> > > -
> > > consumerThread.interrupt();
> > > }
> > >
> > > @@ -201,8 +168,6 @@
> > >
> > > if (message != null) {
> > >
> > > - String[] msgParts = message.split(" ");
> > > -
> > > String priorityStr =
> > > getPriorityStrIfPriority(message);
> > > boolean isPriorityResponse = (priorityStr !=
> > > null);
> > >
> > > @@ -220,9 +185,6 @@
> > > continue; // re-loop to try next msg
> > > }
> > >
> > > - if (msgParts[2].equals("tag"))
> > > - addToInitWorkers((new Integer(msgParts[1])), worker);
> > > -
> > > if (isPriorityResponse) {
> > > unRegisterPriorityWait(priorityStr);
> > > }
> > > @@ -257,10 +219,10 @@
> > >
> > > if (workers.size() < (MAX_WORKERS -
> > > PRIORITY_WORKERS)) {
> > > PluginDebug.debug("Cannot find free worker,
> > > creating worker " + workers.size());
> > > - worker = new PluginMessageHandlerWorker(this, streamHandler,
> > > workers.size(), as, false);
> > > + worker = new PluginMessageHandlerWorker(this, streamHandler,
> > > workers.size(), false);
> > > } else if (prioritized) {
> > > PluginDebug.debug("Cannot find free worker,
> > > creating priority worker " + workers.size());
> > > - worker = new PluginMessageHandlerWorker(this, streamHandler,
> > > workers.size(), as, true);
> > > + worker = new PluginMessageHandlerWorker(this, streamHandler,
> > > workers.size(), true);
> > > } else {
> > > return null;
> > > }
> > > diff -r 738c9c852dda
> > > plugin/icedteanp/java/sun/applet/PluginMessageHandlerWorker.java
> > > ---
> > > a/plugin/icedteanp/java/sun/applet/PluginMessageHandlerWorker.java
> > > Tue Mar 01 11:55:31 2011 -0500
> > > +++
> > > b/plugin/icedteanp/java/sun/applet/PluginMessageHandlerWorker.java
> > > Wed Mar 02 10:28:47 2011 -0500
> > > @@ -40,16 +40,16 @@
> > > class PluginMessageHandlerWorker extends Thread {
> > >
> > > private boolean free = true;
> > > - private boolean isPriorityWorker = false;
> > > - private int id;
> > > - private String message = null;
> > > - PluginStreamHandler streamHandler = null;
> > > - PluginMessageConsumer consumer = null;
> > > + private final boolean isPriorityWorker;
> > > + private final int id;
> > > + private String message;
> > > + private PluginStreamHandler streamHandler;
> > > + private PluginMessageConsumer consumer;
> > >
> > > public PluginMessageHandlerWorker(
> > > PluginMessageConsumer consumer,
> > > PluginStreamHandler streamHandler, int id,
> > > - SecurityManager sm, boolean isPriorityWorker) {
> > > + boolean isPriorityWorker) {
> > >
> > > this.id = id;
> > > this.streamHandler = streamHandler;
> > > @@ -70,7 +70,7 @@
> > >
> > > PluginDebug.debug("Consumer (priority=" +
> > > isPriorityWorker + ") thread " + id + " consuming
> > > " + message);
> > >
> > > - // ideally, whoever returns things object should mark it
> > > + // ideally, whoever returns this object should mark it
> > > // busy first, but just in case..
> > > busy();
> > >
> > > diff -r 738c9c852dda
> > > plugin/icedteanp/java/sun/applet/PluginStreamHandler.java
> > > --- a/plugin/icedteanp/java/sun/applet/PluginStreamHandler.java
> > > Tue Mar 01 11:55:31 2011 -0500
> > > +++ b/plugin/icedteanp/java/sun/applet/PluginStreamHandler.java
> > > Wed Mar 02 10:28:47 2011 -0500
> > > @@ -46,8 +46,6 @@
> > > import java.io.OutputStreamWriter;
> > > import java.net.MalformedURLException;
> > > import java.nio.charset.Charset;
> > > -import java.util.Date;
> > > -import java.util.LinkedList;
> > >
> > > import javax.swing.SwingUtilities;
> > >
> > > @@ -60,27 +58,14 @@
> > >
> > > private JavaConsole console = new JavaConsole();
> > >
> > > - LinkedList<String> writeQueue = new LinkedList<String>();
> > > + private PluginMessageConsumer consumer;
> > > + private Boolean shuttingDown = false;
> > >
> > > - PluginMessageConsumer consumer;
> > > - Boolean shuttingDown = false;
> > > -
> > > - PluginAppletViewer pav;
> > >
> > > public PluginStreamHandler(InputStream inputstream,
> > > OutputStream outputstream)
> > > throws MalformedURLException, IOException {
> > >
> > > PluginDebug.debug("Current context CL=" +
> > > Thread.currentThread().getContextClassLoader());
> > > - try {
> > > - pav = (PluginAppletViewer)
> > > ClassLoader.getSystemClassLoader().loadClass("sun.applet.PluginAppletViewer").newInstance();
> > > - PluginDebug.debug("Loaded: " + pav + " CL=" +
> > > pav.getClass().getClassLoader());
> > > - } catch (InstantiationException e) {
> > > - e.printStackTrace();
> > > - } catch (IllegalAccessException e) {
> > > - e.printStackTrace();
> > > - } catch (ClassNotFoundException e) {
> > > - e.printStackTrace();
> > > - }
> > >
> > > PluginDebug.debug("Creating consumer...");
> > > consumer = new PluginMessageConsumer(this);
> > > @@ -353,17 +338,6 @@
> > > return;
> > > }
> > >
> > > - public boolean messageAvailable() {
> > > - return writeQueue.size() != 0;
> > > - }
> > > -
> > > - public String getMessage() {
> > > - synchronized (writeQueue) {
> > > - String ret = writeQueue.size() > 0 ? writeQueue.poll() : "";
> > > - return ret;
> > > - }
> > > - }
> > > -
> > > private void showConsole() {
> > > SwingUtilities.invokeLater(new Runnable() {
> > > public void run() {
> >
>
> --
> Andrew :)
>
> Free Java Software Engineer
> Red Hat, Inc. (http://www.redhat.com)
>
> Support Free Java!
> Contribute to GNU Classpath and IcedTea
> http://www.gnu.org/software/classpath
> http://icedtea.classpath.org
> PGP Key: F5862A37 (https://keys.indymedia.org/)
> Fingerprint = EA30 D855 D50F 90CD F54D 0698 0713 C3ED F586 2A37
More information about the distro-pkg-dev
mailing list