[15]: RFR(S): 8241718: assert ((klass)->trace_id()) & ((JfrTraceIdEpoch::method_and_class_in_use_this_epoch_bits()))) != 0 in ObjectSampleCheckpoint::add_to_leakp_set

[Markus Gronlund]

Thanks Ioi and Calvin,

I think I found the problem, its related to factorings made to startup as part of  https://bugs.openjdk.java.net/browse/JDK-8233197 .

JFR startup now consist of three stages, compared to the previous two. The third and last stage is where the recordings are launched, i.e. starts JFR. But the check for CDS archive dump is only made as part of stage two.

Will rework and retest the patch to perform the CDS check in both stage two and three, to ensure we are still excluded for the static case as before

For the dynamic case, i.e. to support dumping a CDS archive when JFR is running, I think we would also need the suggestion to remove tag bits using Method::remove_unsharable_info()?

Thanks
Markus


-----Original Message-----
From: Ioi Lam 
Sent: den 22 juni 2020 19:13
To: hotspot-runtime-dev at openjdk.java.net
Subject: Re: [15]: RFR(S): 8241718: assert ((klass)->trace_id()) & ((JfrTraceIdEpoch::method_and_class_in_use_this_epoch_bits()))) != 0 in ObjectSampleCheckpoint::add_to_leakp_set



On 6/22/20 8:32 AM, Calvin Cheung wrote:
> Hi Markus,
>
> On 6/22/20 6:04 AM, Markus Gronlund wrote:
>> Thank you David for taking a look.
>>
>> "I'm wondering though how this arises in practice. I can see this 
>> could be an issue with dynamic archiving as one might expect JFR to 
>> be active when running the app that will do the dynamic archiving.
>> But for a static archive I would (naively perhaps) expect dumping to 
>> disable JFR."
>>
>> Yes, I am a bit unclear on this as well - maybe we can get some 
>> clarification from the CDS/AppCDS folks?
>
> Currently, JFR will be disabled during CDS dumping, both static and 
> dynamic. There's a is_cds_dump_requested function in the following:
>
> http://hg.openjdk.java.net/jdk/jdk/file/8d722fb14093/src/hotspot/share
> /jfr/recorder/jfrRecorder.cpp#l174
>
>
> thanks,
>
> Calvin


I think we just disable part of JFR. If you run test/hotspot/jtreg/runtime/cds/appcds/CDSandJFR.java, this comes out of STDERR


Java HotSpot(TM) 64-Bit Server VM warning: JFR will be disabled during CDS dumping


But STDOUT also says:

0.210s][warning][cds] Skipping jdk/internal/event/Event: Unsupported 
location
[0.210s][warning][cds] Skipping jdk/internal/event/Event: JFR event class
[0.210s][warning][cds] Skipping jdk/jfr/Event: Unsupported location
[0.210s][warning][cds] Skipping jdk/jfr/Event: JFR event class
[0.211s][warning][cds] Skipping GetFlightRecorder$TestEvent: Unsupported 
location
[0.211s][warning][cds] Skipping GetFlightRecorder$TestEvent: JFR event class
[0.211s][warning][cds] Skipping jdk/jfr/events/ActiveRecordingEvent: 
Unsupported location
[0.211s][warning][cds] Skipping jdk/jfr/events/ActiveRecordingEvent: JFR 
event class

We currently have JDK-8222945 -- Support JFR during CDS dumping. We need 
to decide what  to do -- whether we should completely disable JFR during 
CDS dumps, or completely support it, but undo the side effects cleanly.

Thanks
- Ioi



>
>>
>> "Regardless, incidental state changes due to VM configuration at dump 
>> time should not be stored in the archive, so I hope there is nothing 
>> else that we may be storing of a similar nature."
>>
>> I was not aware that Method*'s are treated like this too; I knew from 
>> before that there is a Klass::remove_unshareable_info(), Ioi and I 
>> worked with it many years ago letting it mask off certain bits for 
>> the Klass*. From what I can see, there is also a 
>> ConstantPool::remove_unsharable_info(), but for JFR we don't trace 
>> ConstantPools.
>>
>> If, say, CLDs are incorporated into the CDS archive, we would need to 
>> put in place a similar remove_unsharable_info() for those artifacts 
>> as well.
>>
>> Thank you for the review
>> Markus
>>
>> -----Original Message-----
>> From: David Holmes
>> Sent: den 22 juni 2020 03:23
>> To: Markus Gronlund <markus.gronlund at oracle.com>; 
>> hotspot-runtime-dev at openjdk.java.net
>> Subject: Re: [15]: RFR(S): 8241718: assert ((klass)->trace_id()) & 
>> ((JfrTraceIdEpoch::method_and_class_in_use_this_epoch_bits()))) != 0 
>> in ObjectSampleCheckpoint::add_to_leakp_set
>>
>> Hi Markus,
>>
>> On 22/06/2020 3:44 am, Markus Gronlund wrote:
>>> Greetings,
>>>
>>> Kindly asking for review for the following change set to fix an 
>>> interoperability issue between JFR and CDS.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8241718
>>> Webrev: http://cr.openjdk.java.net/~mgronlun/8241718/webrev00/
>>> Testing: jdk_jfr, runtime/cds
>>>
>>> Summary:
>>> General: Method* trace flags should not be serialized to the CDS 
>>> archive and must therefore be removed in 
>>> Method::remove_unsharable_info().
>>> JFR specific: CLEAR_METHOD_LEAKP(method) removes the leakp flag on 
>>> first method write to have it serialize only once.
>> The fix seems reasonable given the description of the problem.
>>
>> I'm wondering though how this arises in practice. I can see this 
>> could be an issue with dynamic archiving as one might expect JFR to 
>> be active when running the app that will do the dynamic archiving. 
>> But for a static archive I would (naively perhaps) expect dumping to 
>> disable JFR.
>> Regardless, incidental state changes due to VM configuration at dump 
>> time should not be stored in the archive, so I hope there is nothing 
>> else that we may be storing of a similar nature.
>>
>> Thanks,
>> David
>> -----
>>
>>> Thanks
>>> Markus
>>>
>>>
>>>
>>>