RFR: 8340826: Should not send unload notification for scratch classes [v2]

[Markus Grönlund]

On Thu, 26 Sep 2024 18:23:48 GMT, Leonid Mesnik <lmesnik 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.
>
> Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision:
> 
>   space added

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()) {

This is not correct. The unloaded event is generated earlier, this part is the metadata system serializing the constants referenced in said event. The result would still be an event, but it would have missing constants, such as the klass name.

Can scratch klasses short-circuit the "external" unload paths in cases of scratch klasses (which are really only implementation details). There is a property in the classfile parser about "broadcasting" that we (jfr) use to only log a single IK instance when we replace an IK with another schema extended IK.

Logically there should only be one official instance of an IK.

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

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