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

[Aaron Luchko]

On Wed, 2005-06-22 at 14:56 -0700, Keith Seitz wrote:
> 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).

I'm handling this right now, I'm thinking to catch it in run () and
either print the trace and keep looping with our fingers crossed or pull
a shutdown of the jdwp layer at that point.  I don't think we have to
worry about shutting down the whole VM as other VMs I've experimented
with keep running.

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

I haven't really had a chance to look at it yet but this seems like it's
just as usable.

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

Yeah I used a CommandSet[] array with the command set bytes as indexes
in a previous design but got rid of it because I didn't like the gap
from 18-64 (ClassObjectReference and Event), of course this was before I
was familiar enough with the spec to realize Event is never sent by the
debugger and I didn't think to put it back.

I'm putting it back now :)

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

I profiled a debugging session of IBM VM against eclipse using a source
file which had been altered since the compilation (to confuse the
debugger).  In a 3 minute session consisting of about 18000 reply
packets there were 14 that had an error code, 9 of these were
distributed through a block of 1500 replies and none of which were
closer than about 100 replies together.  In every case the error was 101
(ABSENT_INFORMATION).


When I've finished these changes and have played around with the
ByteBuffers a bit I'll resubmit the patch.

thanks for all the feedback!

Aaron