On Mon, 2 Nov 2026 21:28:23 GMT, David Holmes <[email protected]> wrote:
>> A change in JDK-8364258 broke CPU-time sampling for threads that call JNI >> `AttachCurrentThread` or `AttachCurrentThreadAsDaemon` while profiler is >> active. This PR restores the correct behavior, allowing JNI-attached threads >> to be sampled, whether they attach before or after enabling profiler. >> >> When a native thread is attached to the JVM, it goes directly to >> `_thread_in_vm` state, but `JfrThreadLocal::on_start` enables CPU-time >> sampling only for `_thread_new` threads. The fix is to add an explicit check >> for `is_attaching_via_jni()`. >> >> Testing: all `jdk_jfr` tests, including new `TestCPUTimeSamplingJNIAttach`. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > I think there is more that needs fixing here. We should only be calling this > `on_start` method from where we call `Jfr::on_thread_start` - which is during > attach or during the initial part of `run()` for created Java threads. So the > fact we are in this code should mean that we have to enable the sampling > unconditionally. The current logic for new threads seems incorrect because we > call `on_thread_start` after switching to `_thread_in_vm`: > > // Thread is now sufficiently initialized to be handled by the safepoint code > as being > // in the VM. Change thread state from _thread_new to _thread_in_vm > assert(this->thread_state() == _thread_new, "wrong thread state"); > set_thread_state(_thread_in_vm); > > // Before a thread is on the threads list it is always safe, so after > leaving the > // _thread_new we should emit a instruction barrier. The distance to > modified code > // from here is probably far enough, but this is consistent and safe. > OrderAccess::cross_modify_fence(); > > assert(JavaThread::current() == this, "sanity check"); > assert(!Thread::current()->owns_locks(), "sanity check"); > > JFR_ONLY(Jfr::on_thread_start(this);) @dholmes-ora Good point. I also feel it a bit ugly to call `Jfr::on_thread_start` twice per Java thread, but that's what [JDK-8364258](https://github.com/openjdk/jdk/pull/26558) added irrespective of CPU-time sampling, so I didn't really want to touch this code in my fix, which is exclusive to CPU-time profiler. In fact, I don't see anything in `JfrThreadLocal::on_start` that relies on `_thread_new` state, so, for Java threads, we can probably call `on_start` just once from `JavaThread::run()` after switching to `_thread_in_vm`. Initialization of JfrCPUTimeThreadSampling would be then unconditional. Such a refactoring will require https://github.com/openjdk/jdk/pull/33651 to be integrated first. @mgronlun Was there a good reason for adding the second `Jfr::on_thread_start` call? Do you agree with refactoring of your original fix instead of a local 4-lines change as in this PR? ------------- PR Comment: https://git.openjdk.org/jdk/pull/33678#issuecomment-6592475565
