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?
~Aaron
ChangeLog
2005-06-24 Aaron Luchko <[EMAIL PROTECTED]>
* 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 <[EMAIL PROTECTED]>
+ */
+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 21:33:08 -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,52 @@ 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(_connection);
+ _sets[JdwpConstants.CommandSet.ReferenceType.value] = new ReferenceTypeCommandSet(_connection);
+ _sets[JdwpConstants.CommandSet.ClassType.value] = new ClassTypeCommandSet(_connection);
+ _sets[JdwpConstants.CommandSet.ArrayType.value] = new ArrayTypeCommandSet(_connection);
+ _sets[JdwpConstants.CommandSet.InterfaceType.value] = new InterfaceTypeCommandSet(_connection);
+ _sets[JdwpConstants.CommandSet.Method.value] = new MethodCommandSet(_connection);
+ _sets[JdwpConstants.CommandSet.Field.value] = new FieldCommandSet(_connection);
+ _sets[JdwpConstants.CommandSet.ObjectReference.value] = new ObjectReferenceCommandSet(_connection);
+ _sets[JdwpConstants.CommandSet.StringReference.value] = new StringReferenceCommandSet(_connection);
+ _sets[JdwpConstants.CommandSet.ThreadReference.value] = new ThreadReferenceCommandSet(_connection);
+ _sets[JdwpConstants.CommandSet.ThreadGroupReference.value] = new ThreadGroupReferenceCommandSet(_connection);
+ _sets[JdwpConstants.CommandSet.ArrayReference.value] = new ArrayReferenceCommandSet(_connection);
+ _sets[JdwpConstants.CommandSet.ClassLoaderReference.value] = new ClassLoaderReferenceCommandSet(_connection);
+ _sets[JdwpConstants.CommandSet.EventRequest.value] = new EventRequestCommandSet(_connection);
+ _sets[JdwpConstants.CommandSet.StackFrame.value] = new StackFrameCommandSet(_connection);
+ _sets[JdwpConstants.CommandSet.ClassObjectReference.value] = new ClassObjectReferenceCommandSet(_connection);
}
-
+
/**
* 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 +142,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 21:33:08 -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;
}
/**
_______________________________________________
Classpath-patches mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/classpath-patches
