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
_______________________________________________
Classpath-patches mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/classpath-patches
