RFR(XXS): 8225004: Remove invalid assertion in jfr_conditional_flush()

[Markus Gronlund]

Thanks David!

Cheers
Markus

-----Original Message-----
From: David Holmes 
Sent: den 30 maj 2019 07:17
To: Markus Gronlund <markus.gronlund at oracle.com>; hotspot-jfr-dev at openjdk.java.net; Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>
Subject: Re: RFR(XXS): 8225004: Remove invalid assertion in jfr_conditional_flush()

Looks good Markus. Thanks for the clear explanation.

David

On 30/05/2019 3:59 am, Markus Gronlund wrote:
> Greetings,
> 
> Please review this small change to remove an invalid assertion.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8225004
> 
> Change:
> 
> diff -r c1ad2862d0dd -r 0c17f2ea252e src/hotspot/share/jfr/support/jfrFlush.cpp
> --- a/src/hotspot/share/jfr/support/jfrFlush.cpp        Wed May 29 09:53:28 2019 -0700
> +++ b/src/hotspot/share/jfr/support/jfrFlush.cpp        Wed May 29 19:04:34 2019 +0200
> @@ -62,7 +62,6 @@
>   }
> 
>   void jfr_conditional_flush(JfrEventId id, size_t size, Thread* t) {
> -  assert(jfr_is_event_enabled(id), "invariant");
>     if (t->jfr_thread_local()->has_native_buffer()) {
>       JfrStorage::Buffer* const buffer = t->jfr_thread_local()->native_buffer();
>       if (LessThanSize<JfrStorage::Buffer>::evaluate(buffer, size)) {
> 
> 
> Description:
> 
> JfrConditionalFlush(Thread* t) {
>      if (jfr_is_event_enabled(Event::eventId)) { <<--------------- evaluates to true
>        jfr_conditional_flush(Event::eventId, sizeof(Event), t);
>      }
>    }
> 
> void jfr_conditional_flush(JfrEventId id, size_t size, Thread* t) {
>    assert(jfr_is_event_enabled(id), "invariant"); <<-------------------- could evaluate to false
>    if (t->jfr_thread_local()->has_native_buffer()) {
>      JfrStorage::Buffer* const buffer = t->jfr_thread_local()->native_buffer();
>      if (LessThanSize<JfrStorage::Buffer>::evaluate(buffer, size)) {
>        JfrFlush f(buffer, 0, 0, t);
>      }
>    }
> }
> 
> Event settings are updated asynchronously, where event disablement happens after the recording has stopped.
> 
> A thread could have evaluated the initial check to true to enter jfr_conditional_flush(). Depending on visibility, context switching and preemption, another thread could have disabled the event before the thread runs jfr_conditional_flush().
> 
> There is no validity to be asserted at this location, so the assertion needs to be removed.
> 
> Thanks
> Markus
>