RFR: 8340826: Should not send unload notification for scratch classes

[Coleen Phillimore]

On Thu, 26 Sep 2024 12:21:18 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> The jvmti class redefinition creates temporary scratch classes for it's own purposes. These classes are added to corresponding classloaders and might be unloaded.
>> In this case the jvmti/jfr and log events are generated twice: for original class and for it's scratch.
>> 
>> The bug could be reproduced by jfr test
>> jdk/jfr/api/metadata/eventtype/TestUnloadingEventClass.java
>> with '-Xcomp -XX:TieredStopAtLevel=1' or with '-Xcomp'
>> 
>> The test log (modified slightly) shown
>> 
>> 
>> [167.294s][info ][class,unload] unloading class jdk.jfr.api.metadata.eventtype.TestUnloadingEventClass$ToBeUnloaded 0x00000000af1006d8 allocated
>> [167.294s][info ][class,unload] unloading class jdk.jfr.api.metadata.eventtype.TestUnloadingEventClass$ToBeUnloaded 0x00000000af100248 fully_initialized
>> [167.345s][trace][class,unload] unlinking class (subclass): jdk.jfr.api.metadata.eventtype.TestUnloadingEventClass$ToBeUnloaded
>> [167.872s][trace][gc          ] GC(0) Restored 3597 marks, occupying 57552 B
>> [167.924s][info ][gc          ] GC(0) Pause Full (System.gc()) 34M->2M(136M) 691.041ms
>> Unloaded count: 2
>> 
>> 
>> instead of expected
>> 
>> 
>> 
>> [159.737s][info ][class,unload] unloading class jdk.jfr.api.metadata.eventtype.TestUnloadingEventClass$ToBeUnloaded 0x0000000041100248 state: fully_initialized
>> [159.800s][trace][class,unload] unlinking class (subclass): jdk.jfr.api.metadata.eventtype.TestUnloadingEventClass$ToBeUnloaded
>> [160.341s][trace][gc          ] GC(0) Restored 3597 marks, occupying 57552 B
>> [160.384s][info ][gc          ] GC(0) Pause Full (System.gc()) 34M->2M(136M) 710.422ms
>> 
>> 
>> 
>> The test hang because got 2 events while waiting for one.
>> The "allocated" version is the scratch class generated by JVMTI JFR agent that redefine classes.
>> 
>> The fix is to don't send notification for scratch classes. The scratch classes shouldn't have dependency so added assertion. Also, we don't expect any other not loaded classes during unloaded.
>> 
>> Thanks Coleen for details about scratch classed. 
>> 
>> Tested with tier1-5 and with :jdk_jfr with Xcomp and c1.
>
> src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeSet.cpp line 487:
> 
>> 485:   assert(klass != nullptr, "invariant");
>> 486:   assert(_subsystem_callback != nullptr, "invariant");
>> 487:   if(klass->is_instance_klass() && InstanceKlass::cast(klass)->is_scratch_class()) {
> 
> Nit: add a space between if and (.

I'll approve it again after you add this blank.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21166#discussion_r1776967255