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