[RFA/JDWP] IdManager, Events

[Bryce McKinlay]

Hi Keith,

Sorry I havn't got back to you sooner.

Re filtering: I think that whether filtering at the Java level makes a 
significant difference to performance depends on the proportion of 
events that are filtered in a typical debugging session. If, say, 90%+ 
of events are usually filtered out and then discarded, then there might 
be a significant cost in all the object allocations for those events. 
But if filtering isn't all that common, then it shouldn't be a big deal.

Its also worth considering that doing the filtering work will carry a 
significant cost in itself: once we are doing things like matching class 
names against globs, then the cost of generating and then discarding the 
event isn't going to be all that much greater in comparison.

Re: WeakHashMap: basically the issue here is that if we continue to use 
WeakHashMap, it will be possible to have two distinct objects map to the 
same ObjectID. Although perhaps not common, I'm sure that this behaviour 
will cause problems/confusion/incorrect results in some cases and will 
certainly not be consistent with other JWDP implementations.

Here's a solution to consider: While it will still be necessary to 
maintain some hashtable structure for Object->ObjectID mappings, it 
would be relatively trivial to add unique Class IDs and Thread IDs into 
the VMs class and thread structs, thus eliminating the overhead of 
maintain extra tables and looking up the IDs all the time. I suspect 
that this would also be easy to add in other VMs if they don't have it 
already. So, instead of managing the ID<->Object/Class/Thread mappings 
in Java, we could define an interface something along the lines of:

VMDebug.getClassID(java.lang.Class)
VMDebug.getThreadID(java.lang.Thread)
VMDebug.getObjectID(java.lang.Object)
VMDebug.getClass(ClassID)
VMDebug.getThread(ThreadID)
VMDebug.getObject(ObjectID)

This interface could be expanded to provide a lower-level debugging 
interface that bypasses reflection (due to use of reflection operations 
causing class initialization). In turn, this interface could be 
implemented in terms of the JVMTI. We'd need to implement at least a 
significant subset of the JVMTI in libgcj, but that should give you 
everything you need to implement JDWP in one clean, documented 
interface. Note that JVMTI contains the concept of "object tags" which 
seems roughly analogous to JDWP IDs.

In any case, my comments were not intended as a rejection of your patch, 
but rather just to highlight a few issues that I think need to be 
considered/addressed eventually. Its better to get the code in now even 
if its a work in progress that might be changed later. So please go 
ahead and check it in!

Thanks

Bryce


Keith Seitz wrote:

>[pardon top posting]
>
>It's been over a week now, does anyone else have an opinion or does
>everyone agree with Bryce's comments?
>
>Keith
>
>On Mon, 2005-06-27 at 17:51 -0700, Keith Seitz wrote:
>  
>
>>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
>>    
>>
>
>  
>