On Fri, 23 May 2025 21:20:39 GMT, Johannes Bechberger <jbechber...@openjdk.org> wrote:
>> This is the code for the [JEP 509: CPU Time based profiling for >> JFR](https://openjdk.org/jeps/509). >> >> Currently tested using [this test >> suite](https://github.com/parttimenerd/basic-profiler-tests). This runs >> profiles the [Renaissance](https://renaissance.dev/) benchmark with >> - ... different heap sizes >> - ... different GCs >> - ... different samplers (the standard JFR and the new CPU Time Sampler and >> both) >> - ... different JFR recording durations >> - ... different chunk-sizes > > Johannes Bechberger has updated the pull request incrementally with one > additional commit since the last revision: > > Fix compilation @parttimenerd Drive-by comments: The ThreadCrashProtection deeply worries me; I have argued against this technique several times in the past, so I will save us the time here. But I don't see it used anywhere in this PR, so is this a remnant from older versions? The `NoResourceMark` technique does not work as you intend it to work. As a guarantee, this is way too weak. You want to prevent any use of ResourceMark in a non-reentry way. Just guarding chunk allocation is not sufficient since whether or not we hit those asserts at testing time depends on chunk turnover during testing, and that is highly random, very rare, and very badly fuzzied. A customer could experience a much higher frequency of chunk turnovers if they e.g. enable various combination of log flags. You will never have a good test coverage for that. To make the `NoResourceMark` safe, you need to guard every single arena allocation path. But I would guess that is way too expensive even for debug builds. I still think the double-buffering-technique sketched out here https://bugs.openjdk.org/browse/JDK-8349578 is better, and it would be useful outside the context of asynchronous profiling. I'll slot in some days post JDK 25 to explore that. ------------- PR Comment: https://git.openjdk.org/jdk/pull/25302#issuecomment-2907682934
