On Mon, 19 Oct 2026 19:40:45 GMT, Xin Liu <[email protected]> wrote: >> Andrei Pangin has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains two additional >> commits since the last revision: >> >> - Re-implemented native frames support using variable-size requests >> - 8392187: JFR: Use-after-free bug in CPU-time profiler can crash JVM > > src/hotspot/share/jfr/periodic/sampling/jfrThreadSampling.cpp line 390: > >> 388: } >> 389: JfrCPUTimeTraceQueue& queue = tl->cpu_time_jfr_queue(); >> 390: for (u4 offset = 0; offset < queue.size(); ) { > > I think we should use iterator pattern here. > > RequestIterator iter; > while (iter.hasNext()) { > auto& request = iter.next(); > record_cpu_time_thread(request, now, tl, jt, current); > } > > > not only it's easier to read, it also hides variable-length queue and offset.
This is a subjective preference that I don't quite agree with. An iterator is extra code and one more abstraction layer, which is not really justified here given a single instance of a very simple loop. > src/hotspot/share/jfr/recorder/stacktrace/jfrNativeFunctionRepository.cpp > line 28: > >> 26: #include "jfr/recorder/stacktrace/jfrNativeFunctionRepository.hpp" >> 27: #include "jfr/utilities/jfrAllocation.hpp" >> 28: #include "jvm_io.h" > > This header introduces the declaration of jio_ functions. Do we need that in > this cpp file? This is a leftover from debugging; I'll remove it in the next revision. Thanks for finding this. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/32524#discussion_r4320412276 PR Review Comment: https://git.openjdk.org/jdk/pull/32524#discussion_r4320422580
