RFR: 8256811: Delayed/missed jdwp class unloading events [v3]

[Chris Plummer]

On Thu, 23 Jun 2022 12:34:59 GMT, Zhengyu Gu <zgu at openjdk.org> wrote:

>> Currently, jdi only check and process class unloading event when it detects a new GC cycle.
>> 
>> After [JDK-8212879](https://bugs.openjdk.org/browse/JDK-8212879), posting class events can overlap with GC finish event, that results, sometimes, it only captures partial or even empty unloaded class list. The pending list usually can be flushed out at next GC cycle. But for the classes unloaded during the last GC cycle, the class unloading events may lost forever.
>> 
>> This patch checks and processes class unloading events unconditionally, suggested by @kbarrett, the last pending unloaded class list can be flushed by other events, such as `VM_DEATH`.
>> 
>> It also performs `commonRef_compact()` only when there are classes unloaded.
>> 
>> New test failed about 20% without patch, none with patch.
>> 
>> **Update**
>> There are significant changes from early patch. 
>> 
>> The new approach:
>> No longer removing dead objects and post events on VM thread. I believe it was implemented this way to workaround the following issues:
>> - JDI event handler uses JVMTI raw monitor, it requires thread in `_in_native` state
>> - The thread can not hold lock, which is needed to protect `JvmtiTagMap` while walking, when transition to `_in_native` state
>> 
>> The new solution breaks up into two steps:
>> - Collect all dead object tags with lock
>> - Transition to `_in_native` state and post object free events in one batch
>> 
>> This way, JDI event handler can process object free events upon arrivals without delay.
>
> Zhengyu Gu has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 18 additional commits since the last revision:
> 
>  - Improve naming and cleanup
>  - Merge branch 'master' into JDK-8256811-jdi-missing-class-unloading-event
>  - v4
>  - v3
>  - v2
>  - Merge branch 'master' into JDK-8256811-jdi-missing-class-unloading-event
>  - Merge branch 'jdi_tmp' into JDK-8256811-jdi-missing-class-unloading-event
>  - v0
>  - v2
>  - v1
>  - ... and 8 more: https://git.openjdk.org/jdk/compare/5863bf43...559b4bf1

I think @coleenp  and @sspitsyn should review the JVMTI changes.

One thing I want to point out is I believe your JVMTI changes mean that the debug agent now gets the ObjectFree events on the Service Thread. At first I was worried this might lead to strange suspend/resume issues of this thread, but I think now it is ok. Let me step through why here.

I don't think the Service Thread is a thread that normally even shows up in the debugger's list of java threads. Also, there's no way for the debugger to call ThreadReference.resume() on it, because unlike all other events, there is no ThreadReference included with the ClassUnload event. EventSet.resume() also cannot resume just the Service Thread. However, posting of the CLASS_UNLOAD event eventually leads to (even before your changes):

``` 
        reportEvents(env, eventSessionID, (jthread)NULL, 0,
                            (jclass)NULL, (jmethodID)NULL, 0, eventBag); 


So the thread provided is NULL. Looking then at `handleReportEventCompositeCommand()`, it looks like if the suspend policy for the event is not NONE, then we do a suspendAll because thread == NULL.  If the debugger specified SUSPEND_THREAD as the policy, they will get a SUSPEND_ALL. So it looks like there is no issue with there being an explicit suspend of the Service Thread, and no behavior change here due to your changes. What I'm not sure of is if the Service Thread will get suspended when the debug agent does a Suspend All, but that question is moot, because if it is, it also was before  your changes, so you aren't introducing anything new in this respect.

src/hotspot/share/prims/jvmtiExport.cpp line 1702:

> 1700:   } else {
> 1701:     post_object_free_on_java_thread(env, objects);
> 1702:   }

Can you explain why sometimes it is a VMThread and why sometimes it is a JavaThread?

src/jdk.jdwp.agent/share/native/libjdwp/classTrack.c line 52:

> 50: 
> 51: /*
> 52:  * Add a class to the prepared class table.

This comment is out of date. There is no longer a table and hasn't been for a while. I'd suggest a comment about tagging it so we know when the class is freed. Aslo, maybe `classTrack_trackPreparedClass()` would be a better name.

src/jdk.jdwp.agent/share/native/libjdwp/classTrack.c line 156:

> 154: void
> 155: classTrack_activate(JNIEnv *env)
> 156: { }

Remove

src/jdk.jdwp.agent/share/native/libjdwp/classTrack.c line 165:

> 163: classTrack_reset(void)
> 164: {}
> 165: 

Remove

src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c line 462:

> 460: /* Create a synthetic class unload event for every class no longer present.
> 461:  * Analogous to event_callback combined with a handler in a unload specific
> 462:  * (no event structure) kind of way.

This comment was already out of date. It is based on when we took a new snapshot of loaded classes and compared it with the old snapshot to determine which ones were just unloaded. I would suggest change it to just say something like "Create a synthetic class unload event for the specified signature."

src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c line 627:

> 625: 
> 626:     if (evinfo->ei == EI_CLASS_UNLOAD) {
> 627:         synthesizeUnloadEvent((char*)jlong_to_ptr(evinfo->tag), env);

You're in uncharted waters here. Previously `event_callback()` was always called with a `thread` argument except for VMDeath. Now it is being called during an ObjectFree, which has no thread associated with it. That means all the code below that is executed when we have a thread is no longer being executed after the ClassUnload events are generated by the `synthesizeUnloadEvent()` call. Have you thought this part through?

My guess is that for the most part it is harmless. The "invoke" support is not relevant in this case. I think/hope that means there is no need for calling `threadControl_onEventHandlerEntry()`.  The `filterAndHandleEvent()` probably amounts to an (expensive) nop, although it seems silly to be calling it for the `ObjectFree` event. I'd suggest just returning at this point in the code, but you do still have the restoration of the pending exception to handle. Maybe that should just be done here and then return. It will make it a lot more clear that the rest of the code below is not needed.

src/jdk.jdwp.agent/share/native/libjdwp/eventHandler.c line 966:

> 964:  */
> 965: void JNICALL
> 966: cbTrackingObjectFree(jvmtiEnv* jvmti_env, jlong tag)

I think it's misleading to have this event handler here since it is handled very differently than all the other events, and with a different `jvmti_env`. Perhaps move this back to classTrack.c, and instead of calling `event_callback()`, call something that just does the parts of `event_callback()` that are really needed for `ObjectFree` (see my comments in `event_callback()` also). What might work bests is to export `synthesizeUnloadEvent()` and just call it from this callback (after relocating it). There's not much of `event_handler()` you need other than the call to `synthesizeUnloadEvent()`.

-------------

PR: https://git.openjdk.org/jdk/pull/9168