On Mon, 2005-06-27 at 17:03 -0400, Bryce McKinlay wrote:
Please post ChangeLog entries whenever you post a patch. I've made some
comments below.
ChangeLog's were posted with the original patches, and their all the
same "* filename.java: New file." I assumed such a trivial thing would
be far too silly to repost. I am incorrect, and I shall simply post
links to everything next time.
It seems like most JDWP events will be generated at the VM level. So,
rather than implementing event filtering here, I think it would be
better to pass the event filter through to the VM and have it implement
filtering right at the source. This would be more efficient because the
un-needed events would never be generated, instead of being created and
then being discarded by the Java JDWP code.
There are reasons for doing it this way, though. The biggest one is code
re-use, since any VM wanting to use JDWP support will need to implement
exactly the same thing.
In any case, please allow me to walk through a simple example of class
preparation notification to see if we can find a better solution.
For class prepare events, the debugger can specify the following
notification parameters: ignore count, thread, class, and class name
glob (matching and not matching).
Regardless of how/where filtering is performed, the VM must generate/get
IDs for thread and class, compute any "hit counts", and get the string
representation of the class name (if it does not have that data
available).
That's a whole lot of work already mandated by filtering. IMO pushing
filtering down into the VM is not going to provide sufficient incentive
to outweigh re-usability.
The only alternative solution I could devise involved looking up, e.g.,
the Thread when an event request comes in, storing a Soft/WeakReference
to that Object and then comparing that against the Thread supplied with
the event notification. I guess there is sufficient incentive to move to
this, even though I had misgivings about this earlier (but it's been so
long, I don't recall why).
Nonetheless, none of this addresses your concerns.
Perhaps a bigger piece of the puzzle might help elide a new solution?
For ClassPrepareEvent, I have a notification in gcj which looks
something like (pardon lots of pseudo-code):
if (gnu::classpath::jdwp::Jdwp::isDebugging)
{
using namespace gnu::classpath::jdwp;
java::lang::Thread* t = currentThread();
java::lang::Class* clazz = defining class;
int flags = class preparation flags;
event::ClassPrepareEvent* event = new event::ClassPrepareEvent (t, clazz,
flags);
Jdwp.notify (event);
}
Jdwp.notify actually attempts to ascertain what request from the
debugger actually wanted the notification. If no registered notification
exists, the event is never converted into bytes and sent. Otherwise, the
request ID is returned and the event is converted to bytes and sent over
the transport.
I'm afraid I just don't see where any appreciable performance gain is
going to be made, other than running native+JNI vs. (possibly)
interpreted java.
Note that WeakHashMap matches keys based on equals(), not object
identity. This might not be what you want for a debugger, since two
"equal" but different objects may be an important distinction for the
user. Of course, IdentityHashMap isn't what we want either because the
references won't be weak. I think that some custom data structure is
needed here.
Then again, perhaps this isn't so important for all cases, so I wouldn't
be against this patch if you want to commit this as-is in the interim,
then come back and fix it later.
I don't really know. I don't recall reading anything in the JDWP
specification alluding to a distinction. If worse comes to worse, we can
revive and revise the ReferenceKey patch and make it work.
Keith