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

[Markus Gronlund]

Thank you Dan,

Appreciate you taking a look.

Thanks again
Markus

-----Original Message-----
From: Daniel D. Daugherty 
Sent: den 29 maj 2019 20:39
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()

On 5/29/19 1:59 PM, 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)) {

Thumbs up.

And thanks for the analysis below. If I remember correctly, JVM/TI async events and event handlers run into the same issue. :-)

Dan


>
>
> 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