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

[Bryce McKinlay]

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.

Regards,

Bryce