[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[cp-patches] Re: [RFA/JDWP] CommandSet interface and PacketProcessor
From: |
Aaron Luchko |
Subject: |
[cp-patches] Re: [RFA/JDWP] CommandSet interface and PacketProcessor |
Date: |
Fri, 24 Jun 2005 18:49:55 -0400 |
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