[cp-patches] Re: [RFA/JDWP] CommandSet interface and PacketProcessor

[Aaron Luchko]

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  <address@hidden>

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

jdwp_commandsets2.patch
Description: Text Data