Re: [RFA/JDWP] IdManager, Events

[Bryce McKinlay <mckinlay at redhat dot com>]

Keith Seitz wrote:

> So, after much discussion last week, I'm not sure where I stand in
> making any progress at getting this stuff committed.
>
> I've attached the outstanding patches awaiting more review and/or
> approval.
>  
Hi Keith,

Please post ChangeLog entries whenever you post a patch. I've made some 
comments below.

> +public interface IEventFilter
> +{
> +  /**
> +   * Does the given event match the filter?
> +   *
> +   * @param event  the <code>Event</code> to scrutinize
> +   */
> +  public boolean matches (Event event);
> +}
>  

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.

> +/**
> + * This class manages objects and referencetypes that are reported
> + * to the debugger. All objects and referencetypes reported to the
> + * debugger should go through this manager.
> + *
> + * @author Keith Seitz  (keiths@redhat.com)
> + */
> +public class IdManager
> +{
> +  // Reference queues for invalid ObjectIds and ReferenceTypeIds
> +  private ReferenceQueue _oidRefQueue;
> +  private ReferenceQueue _ridRefQueue;
> +
> +  /* Mapping of objects (Object) to IDs (ObjectId). Table is a
> +     synchronized WeakHashMap. */
> +  private Map _oidTable;
>  

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.

Bryce