This is the mail archive of the
java-patches@gcc.gnu.org
mailing list for the Java project.
Re: [RFA/JDWP] CommandSet interface and PacketProcessor
On Fri, 2005-06-24 at 17:40 -0400, Aaron Luchko wrote:
> On Wed, 2005-06-22 at 16:07 -0400, Bryce McKinlay wrote:
> > Aaron Luchko wrote:
> >
> > >Hello, I've been working with Keith Seitz to help with jdwp. This patch
> > >adds the functionality for PacketProcessor to act on a given packet from
> > >the debugger via CommandSet objects along with the CommandSet interface.
> > >
> > >
> > >+ try
> > >+ {
> > >+ try
> > >+ {
> > >+ if (set != null)
> > >+ {
> > >+ _shutdown = set.runCommand(distr, ostr, command);
> > >+ reply.setData(bytes.toByteArray());
> > >+ }
> > >+ else
> > >+ {
> > >+ // This command set wasn't in our tree
> > >+ reply.setErrorCode(JdwpConstants.Error.NOT_IMPLEMENTED);
> > >+ }
> > >+ }
> > >+ catch (JdwpException ex)
> > >+ {
> > >+ reply.setErrorCode(ex.getErrorCode ());
> > >+ }
> > >+ _connection.sendPacket (reply);
> > >+ }
> > >+ catch (IOException ioe)
> > >+ {
> > >+ // Not much we can do...
> > >+ }
> > > }
> > >
> > >
> >
> > Hi Aaron,
> >
> > I know you didn't introduce this, but it is bad practice to catch and
> > silently ignore exceptions like this. This can result in hard-to-find
> > bugs because in the event of an error, this code will keep cycling
> > through the packet loop silently instead of failing in some obvious way
> > at the point where the error occurred.
> >
> > Either _processOnePacket() needs to be declared as "throws IOException",
> > and the exception handled some at the top level, or you could rethrow
> > the IOException as some other exception type.
> >
> > In addition, I have a few potential performance concerns about this code:
> >
> > 1. You are creating a new set of "temporary" streams for every packet
> > that is sent. It would be much better to reuse the same streams for all
> > packets, if they are necessary, but I suspect you can get away with
> > eliminating most of them by refactoring the design a little.
> >
> > 2. Since you need to look up a CommandSet object for each packet that is
> > sent, and the command sets appear to be singletons, it might make more
> > sense to reference the CommandSet objects directly from
> > JdwpCommandPacket rather than constructing a Byte and doing a hashtable
> > look-up each time. You could also, perhaps, avoid the hashtable by
> > maintaining an array, indexed by the JDWP command byte, mapping to the
> > appropriate CommandSet object.
> >
> > 3. As a general design comment, there are potential performance issues
> > with using exceptions to represent the JDWP error codes, if these errors
> > are likely to occur during "normal" use of the JDWP engine. Exceptions
> > should be avoided as part of normal program flow control because they
> > are slow. Even Sun advises this, but exceptions under GCJ are,
> > unfortunately, particularly slow. If these are likely to occur
> > frequently while debugging, then encapsulating them in some result-code
> > object, instead of exceptions, should be considered.
> >
> > We don't need to go overboard on hunting performance nits, however there
> > are a few simple fixes to be made here that will greately reduce the
> > overhead of debugging on a running application.
>
> Ok, after some discussions with Bryce and Keith I've arrived at an
> improved version of the patch. The first couple changes involve using an
> array instead of the hashtable and tossing the IOException up from
> _processOnePacket and exiting in run() when we get it.
>
> The final changes involve the streams used to read the command packet
> data and write the reply packet data. The data portion of the command
> packet is now wrapped in a ByteBuffer. For the reply packet however the
> size of the data portion is simply too variable so we had to stick with
> a DataInputStream, the constructors however have been moved out and the
> only action required between packets is a reset() call.
>
> This also adds a simple convenience constructor to JdwpReplyPacket.
>
> Comments on the new implementation?
The CommandSet objects don't actually need a JdwpConnection at all so
I've removed them from the where I construct the array and fixed the
line length too.
Aaron
ChangeLog
2005-06-24 Aaron Luchko <aluchko@redhat.com>
* gnu/classpath/jdwp/processor/CommandSet.java: New file.
* gnu/classpath/jdwp/processor/PacketProcessor.java: Use
CommandSets to handle JdwpCommandPackets.
* gnu/classpath/jdwp/transport/JdwpReplyPacket.java: New
Constructor.
--- /dev/null 2005-06-09 16:29:11.371620296 -0400
+++ gnu/classpath/jdwp/processor/CommandSet.java 2005-06-24 17:28:47.000000000 -0400
@@ -0,0 +1,68 @@
+/* CommandSet.java -- An interface defining JDWP Command Sets
+ Copyright (C) 2005 Free Software Foundation
+
+This file is part of GNU Classpath.
+
+GNU Classpath is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 2, or (at your option)
+any later version.
+
+GNU Classpath is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GNU Classpath; see the file COPYING. If not, write to the
+Free Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+02111-1307 USA.
+
+Linking this library statically or dynamically with other modules is
+making a combined work based on this library. Thus, the terms and
+conditions of the GNU General Public License cover the whole
+combination.
+
+As a special exception, the copyright holders of this library give you
+permission to link this library with independent modules to produce an
+executable, regardless of the license terms of these independent
+modules, and to copy and distribute the resulting executable under
+terms of your choice, provided that you also meet, for each linked
+terms of your choice, provided that you also meet, for each linked
+independent module, the terms and conditions of the license of that
+module. An independent module is a module which is not derived from
+or based on this library. If you modify this library, you may extend
+this exception to your version of the library, but you are not
+obligated to do so. If you do not wish to do so, delete this
+exception statement from your version. */
+
+
+package gnu.classpath.jdwp.processor;
+
+import gnu.classpath.jdwp.exception.JdwpException;
+
+import java.io.DataOutputStream;
+import java.nio.ByteBuffer;
+
+/**
+ * A class representing a JDWP Command Set. This class serves as a generic
+ * interface for all Command Sets types used by JDWP.
+ *
+ * @author Aaron Luchko <aluchko@redhat.com>
+ */
+public interface CommandSet
+{
+ /**
+ * Runs the given command with the data in distr and writes the data for the
+ * reply packet to ostr.
+ *
+ * @param bb holds the data portion of the Command Packet
+ * @param os data portion of the Reply Packet will be written here
+ * @param command the command field of the Command Packet
+ * @return true if the JDWP layer should shut down in response to this packet
+ * @throws JdwpException command wasn't carried out successfully
+ */
+ public boolean runCommand(ByteBuffer bb, DataOutputStream os,
+ byte command)
+ throws JdwpException;
+}
? gnu/classpath/jdwp/processor/CommandSet.java
Index: gnu/classpath/jdwp/processor/PacketProcessor.java
===================================================================
RCS file: /cvs/gcc/gcc/libjava/gnu/classpath/jdwp/processor/PacketProcessor.java,v
retrieving revision 1.1
diff -u -p -u -r1.1 PacketProcessor.java
--- gnu/classpath/jdwp/processor/PacketProcessor.java 15 Jun 2005 03:10:31 -0000 1.1
+++ gnu/classpath/jdwp/processor/PacketProcessor.java 24 Jun 2005 22:46:58 -0000
@@ -40,15 +40,17 @@ exception statement from your version. *
package gnu.classpath.jdwp.processor;
-import gnu.classpath.jdwp.Jdwp;
-import gnu.classpath.jdwp.event.Event;
+import gnu.classpath.jdwp.JdwpConstants;
import gnu.classpath.jdwp.exception.JdwpException;
-import gnu.classpath.jdwp.transport.JdwpConnection;
import gnu.classpath.jdwp.transport.JdwpCommandPacket;
+import gnu.classpath.jdwp.transport.JdwpConnection;
import gnu.classpath.jdwp.transport.JdwpPacket;
import gnu.classpath.jdwp.transport.JdwpReplyPacket;
+import java.io.ByteArrayOutputStream;
+import java.io.DataOutputStream;
import java.io.IOException;
+import java.nio.ByteBuffer;
/**
* This class is responsible for processing packets from the
@@ -66,7 +68,16 @@ public class PacketProcessor
// Shutdown this thread?
private boolean _shutdown;
+
+ // A Mapping of the command set (Byte) to the specific CommandSet
+ private CommandSet[] _sets;
+
+ // Contents of the ReplyPackets data field
+ private ByteArrayOutputStream _outputBytes;
+ // Output stream around _outputBytes
+ private DataOutputStream _os;
+
/**
* Constructs a new <code>PacketProcessor</code> object
* Connection must be validated before getting here!
@@ -77,20 +88,68 @@ public class PacketProcessor
{
_connection = con;
_shutdown = false;
+
+ // MAXIMUM is the value of the largest command set we may receive
+ _sets = new CommandSet[JdwpConstants.CommandSet.MAXIMUM + 1];
+ _outputBytes = new ByteArrayOutputStream();
+ _os = new DataOutputStream (_outputBytes);
+
+ // Create all the Command Sets and add them to our array
+ _sets[JdwpConstants.CommandSet.VirtualMachine.value] =
+ new VirtualMachineCommandSet();
+ _sets[JdwpConstants.CommandSet.ReferenceType.value] =
+ new ReferenceTypeCommandSet();
+ _sets[JdwpConstants.CommandSet.ClassType.value] =
+ new ClassTypeCommandSet();
+ _sets[JdwpConstants.CommandSet.ArrayType.value] =
+ new ArrayTypeCommandSet();
+ _sets[JdwpConstants.CommandSet.InterfaceType.value] =
+ new InterfaceTypeCommandSet();
+ _sets[JdwpConstants.CommandSet.Method.value] =
+ new MethodCommandSet();
+ _sets[JdwpConstants.CommandSet.Field.value] =
+ new FieldCommandSet();
+ _sets[JdwpConstants.CommandSet.ObjectReference.value] =
+ new ObjectReferenceCommandSet();
+ _sets[JdwpConstants.CommandSet.StringReference.value] =
+ new StringReferenceCommandSet();
+ _sets[JdwpConstants.CommandSet.ThreadReference.value] =
+ new ThreadReferenceCommandSet();
+ _sets[JdwpConstants.CommandSet.ThreadGroupReference.value] =
+ new ThreadGroupReferenceCommandSet();
+ _sets[JdwpConstants.CommandSet.ArrayReference.value] =
+ new ArrayReferenceCommandSet();
+ _sets[JdwpConstants.CommandSet.ClassLoaderReference.value] =
+ new ClassLoaderReferenceCommandSet();
+ _sets[JdwpConstants.CommandSet.EventRequest.value] =
+ new EventRequestCommandSet();
+ _sets[JdwpConstants.CommandSet.StackFrame.value] =
+ new StackFrameCommandSet();
+ _sets[JdwpConstants.CommandSet.ClassObjectReference.value] =
+ new ClassObjectReferenceCommandSet();
}
-
+
/**
* Main run routine for this thread. Will loop getting packets
* from the connection and processing them.
*/
public void run ()
{
- while (!_shutdown)
+ try
{
- _processOnePacket ();
+ while (!_shutdown)
+ {
+ _processOnePacket ();
+ }
}
+ catch (IOException ex)
+ {
+ ex.printStackTrace();
+ }
+ // Time to shutdown, tell the _connection thread to stop reading
+ _connection.shutdown();
}
-
+
/**
* Shutdown the packet processor
*/
@@ -99,40 +158,58 @@ public class PacketProcessor
_shutdown = true;
interrupt ();
}
-
+
// Helper function which actually does all the work of waiting
// for a packet and getting it processed.
private void _processOnePacket ()
+ throws IOException
{
JdwpPacket pkt = _connection.getPacket ();
- if (pkt instanceof JdwpReplyPacket)
+
+ if (!(pkt instanceof JdwpCommandPacket))
{
- // We're not supposed to get these from the debugger!
- // Drop it on the floor
- return;
+ // We're not supposed to get these from the debugger!
+ // Drop it on the floor
+ return;
}
-
+
if (pkt != null)
{
- JdwpReplyPacket reply;
- try
- {
- // !! process packet here !!
- reply = new JdwpReplyPacket (pkt, (short) 0);
- }
- catch (JdwpException ex)
- {
- reply = new JdwpReplyPacket (pkt, ex.getErrorCode ());
- }
-
- try
- {
- _connection.sendPacket (reply);
- }
- catch (IOException ioe)
- {
- // Not much we can do...
- }
+ JdwpCommandPacket commandPkt = (JdwpCommandPacket) pkt;
+ JdwpReplyPacket reply = new JdwpReplyPacket(commandPkt);
+
+ // Reset our output stream
+ _outputBytes.reset();
+
+ // Create a ByteBuffer around the command packet
+ ByteBuffer bb = ByteBuffer.wrap(commandPkt.getData());
+ byte command = commandPkt.getCommand();
+ byte commandSet = commandPkt.getCommandSet();
+
+ CommandSet set = null;
+ try
+ {
+ // There is no command set with value 0
+ if (commandSet > 0 && commandSet < _sets.length)
+ {
+ set = _sets[commandPkt.getCommandSet()];
+ }
+ if (set != null)
+ {
+ _shutdown = set.runCommand(bb, _os, command);
+ reply.setData(_outputBytes.toByteArray());
+ }
+ else
+ {
+ // This command set wasn't in our tree
+ reply.setErrorCode(JdwpConstants.Error.NOT_IMPLEMENTED);
+ }
+ }
+ catch (JdwpException ex)
+ {
+ reply.setErrorCode(ex.getErrorCode ());
+ }
+ _connection.sendPacket (reply);
}
}
}
Index: gnu/classpath/jdwp/transport/JdwpReplyPacket.java
===================================================================
RCS file: /cvs/gcc/gcc/libjava/gnu/classpath/jdwp/transport/JdwpReplyPacket.java,v
retrieving revision 1.1
diff -u -p -u -r1.1 JdwpReplyPacket.java
--- gnu/classpath/jdwp/transport/JdwpReplyPacket.java 1 Jun 2005 20:04:05 -0000 1.1
+++ gnu/classpath/jdwp/transport/JdwpReplyPacket.java 24 Jun 2005 22:46:58 -0000
@@ -75,9 +75,20 @@ public class JdwpReplyPacket extends Jdw
*/
public JdwpReplyPacket (JdwpPacket pkt, short errorCode)
{
+ this(pkt);
+ _errorCode = errorCode;
+ }
+
+ /**
+ * Constructs a <code>JdwpReplyPacket</code> with the
+ * id from the given packet and an empty error code
+ *
+ * @param pkt the packet whose id this packet will use
+ */
+ public JdwpReplyPacket (JdwpPacket pkt)
+ {
super (pkt);
_flags = (byte) JDWP_FLAG_REPLY;
- _errorCode = errorCode;
}
/**