[cp-patches] Re: [RFA/JDWP] IdManager, Events

[Keith Seitz]

[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