[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: |
Wed, 22 Jun 2005 18:37:31 -0400 |
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