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

[Chris Plummer]

On Fri, 1 Jul 2022 12:57:55 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.
>> 
>> **Update 2**
>> There is a comment for ` JvmtiTagMap::check_hashmap()` that states `ObjectFree` events are posted before heap walks.
>> 
>> // This checks for posting and rehashing before operations that
>> // this tagmap table.  The calls from a JavaThread only rehash, posting is
>> // only done before heap walks.
>> void JvmtiTagMap::check_hashmap(bool post_events) {
>> 
>> Now, the events are actually posted after heap walks, but I don't think it makes any material material difference. 
>> Even the events are posted earlier in old code, but they are only processed after next GC cycle.
>
> Zhengyu Gu has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Incorporated plummercj's test changes

test/jdk/com/sun/jdi/TestClassUnloadEvents.java line 61:

> 59:                     return;
> 60:                 }
> 61:             }

I know you were having issues with the test sometimes failing and asked about if the GC's are deterministic, I assume this is the work around for that. My concern is if something were to change and the test started to never get any ClassUnloadEvents, that would go unnoticed. I'd rather you made it so the test failed after exiting this loop, and bump up MAX_RETRY to a high enough number so that we are unlikely to ever see that happen.

You should also have a look at the runtime/ClassUnload/UnloadTest.java and see how it handles class unloading. It relies on the ClassUnloadCommon library class. One difference I see there is that before doing a System.gc(), it first allocates 16m of memory in 1k chunks to force a young gen collection. I don't understand details of GC collection, but perhaps System.gc() is simply promoting from the youngGen and therefore not freeing any Class instance found there, so you need to promote first and then do the full GC.

test/jdk/com/sun/jdi/TestClassUnloadEvents.java line 182:

> 180:                 eventSet.resume();
> 181:             }
> 182:         } catch (InterruptedException | VMCannotBeModifiedException | IOException | IllegalConnectorArgumentsException | VMStartException e) {

I think you should remove the try/catch and just declare that this method throws Exception. That seems to be what other tests are doing.

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

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