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

[Aaron Luchko]

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