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

[Calvin Cheung]

+1

thanks,

Calvin

On 6/23/20 2:46 PM, Ioi Lam wrote:
> Hi Markus,
>
> This new version looks good to me.
>
> Thanks
> - Ioi
>
> On 6/23/20 9:17 AM, Markus Gronlund wrote:
>> 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
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>