RFR: 8352738: Implement JEP 520: JFR Method Timing and Tracing

[Markus Grönlund]

On Thu, 22 May 2025 10:44:09 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Could I have a review of this enhancement that will add tracing capabilities to JFR? There are opportunities for performance improvements in the implementation, but I would rather add them later and separately.
>> 
>> Testing: tier 1-3, test/jdk/jdk/jfr
>> 
>> Thanks
>> Erik
>
> src/hotspot/share/jfr/instrumentation/jfrClassTransformer.cpp line 47:
> 
>> 45: #include "utilities/macros.hpp"
>> 46: 
>> 47: static void log_pending_exception(oop throwable) {
> 
> Put it under `JfrClassTransformer::`? Global symbols would eventually clash.

Its static so not a globally visible symbol.

> src/hotspot/share/jfr/support/methodtracer/jfrFilterManager.cpp line 81:
> 
>> 79: 
>> 80: const JfrFilter* JfrFilterManager::current() {
>> 81:   return Atomic::load(&_current);
> 
> Memory ordering question again: does it need `load_acquire`? For example, are we accessing `JfrFilter` contents from another thread? `xchg` installation covers the release part, we only need need `load_acquire` here.

Atomic::load_acquire() it should be, that's right - thanks.

> src/hotspot/share/oops/instanceKlass.hpp line 360:
> 
>> 358:   Method* method_with_idnum(int idnum) const;
>> 359:   Method* method_with_orig_idnum(int idnum) const;
>> 360:   Method* method_with_orig_idnum(int idnum, int version) const;
> 
> Checking: do you need these turned `const`, or this is just convenient touchup? If this is unnecessary, I would probably limit the changes to shared code.

The change is correct to improve const-correctness. If you work with a const Method*, then this is required.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/25306#discussion_r2104679792
PR Review Comment: https://git.openjdk.org/jdk/pull/25306#discussion_r2104677815
PR Review Comment: https://git.openjdk.org/jdk/pull/25306#discussion_r2104676648