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