[RFA/JDWP] CommandSet interface and PacketProcessor

[Keith Seitz]

On Wed, 2005-06-22 at 16:07 -0400, Bryce McKinlay wrote:

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

That was my bad. You are correct, that exception probably should get
thrown higher up and compared for a fatal condition (like the connection
is no longer valid).

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

As recommended by someone else, perhaps it would be better to have a
ByteBuffer allocated inside the packet processor for this purpose? Do
you think that would that be sufficiently better?

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

That's a great recommendation. I have done this, too, somewhere, and I
intended to revisit this very issue. I will rewrite that code with this
in mind before submitting it for approval.

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

That was my decision, and perhaps I should explain why I chose to do it
that way. I consider the debugger a programmatic extension of the VM,
albeit operating in its own VM and communicating with the inferior VM
via a socket and JDWP.

When the debugger does something to generate one of these exceptions, it
is very likely a programming error in the debugger. For example,
specifying a negative ignore count on a breakpoint is quite simply a
programming error in the debugger: it did not validate its input.

Like any Exception, these are definitely NOT "normal" paths. [Again,
IMO.]

However, it would not surprise me to see debuggers rely on these things
(though I once thought otherwise), even though the captured debug
sessions I have between Eclipse and the IBM VM show no error replies
(except VM_DEAD at the end).

Like much of the JDWP "specification", time will ultimately tell whether
to rethink this decision.

I appreciate your comments, and welcome your input on the patches I have
submitted (and will submit). [Have you looked at IdManager, Event, and
IEventFilter?]

I've learned a lot more about Java in the last few weeks than I ever
expected.

Keith