[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 markus.gronlund at oracle.com
Tue Jun 23 16:17:34 UTC 2020


Hi again,

Turns out this is a bit more complex that we originally thought.

I tried to only add the check to JfrRecorder::on_create_vm_3() as well. Unfortunately this is not sufficient to solve this problem. Why? Because it will only cover the cases where a recording is requested explicitly on the command-line. There are numerous entry points to start JFR - command-line, jcmd (covered by the check in JfrRecorder::on_create_vm_2() as no DCMDs gets registered) but we also have the programmatic Java API to account for. This could be used by an application or a JVMTI agent. 

And this is what happens in the situation related to JDK- 8241718. JFR is programmatically started, which causes the methods to get tagged before they are saved to the CDS archive. One could try to disallow the Java API from starting JFR if the CDS dump flag is set, but this is harder and can cause problems for applications expecting this to work. The longer term solution is the dynamic case, i.e. to have the CDS archive be created with JFR running, so this suggestion is a step towards that. Therefore, we add the entry to Method::remove_unsharable_info().

Updated webrev: http://cr.openjdk.java.net/~mgronlun/8241718/webrev01/

I added the check to JfrRecorder::on_create_vm_3() to cover the static case. The removal of the second conjunct in is_cds_dump_requested() is to avoid JfrOptionSet::configure() to make an upcall to Java if there are global JFR flags set,  as JfrOptionSet::start_flight_recording_options() only checks local, or recording specific options.

Thanks
Markus



-----Original Message-----
From: Ioi Lam 
Sent: den 23 juni 2020 17:19
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



On 6/22/20 10:57 AM, Markus Gronlund wrote:
> 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()?
Actually, today when either static or dynamic CDS dumping is happening,
Arguments::is_dumping_archive() will return true:

static bool is_cds_dump_requested() {
   // we will not be able to launch recordings if a cds dump is being requested
   if (Arguments::is_dumping_archive() &&
(JfrOptionSet::start_flight_recording_options() != NULL)) {
     warning("JFR will be disabled during CDS dumping");
     teardown_startup_support();
     return true;
   }
   return false;
}

So I think you don't need to do special handling for dynamic dumping for now.

Thanks
- Ioi

>
> 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/shar
>> e
>> /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
>>>>
>>>>
>>>>
>>>>



More information about the hotspot-runtime-dev mailing list