RFR: 8348402: PerfDataManager stalls shutdown for 1ms [v4]

[Patricio Chilano Mateo]

On Fri, 31 Jan 2025 08:30:29 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Found this when studying Leyden performance. [JDK-8049304](https://bugs.openjdk.org/browse/JDK-8049304) added 1ms sleep on destruction path to catch up with threads updating the counters. 
>> 
>> I was not able to confidently prove the deletion race is benign. Even though `sleep` is not really a fix for that race either, I think it is safer to use GlobalCounter to coordinate deletions.
>> 
>> This improves startup (roundtrip) tests for the expected 1ms:
>> 
>> 
>> $ hyperfine -w 10 -r 100 ...
>> 
>> # Before
>> Benchmark 1: build/linux-x86_64-server-release/images/jdk/bin/java -XX:+UnlockExperimentalVMOptions -XX:+UseEpsilonGC -Xmx128m Hello
>>   Time (mean ± σ):      20.0 ms ±   0.3 ms    [User: 11.7 ms, System: 16.0 ms]
>>   Range (min … max):    19.0 ms …  21.7 ms    1000 runs
>> 
>> # After
>> Benchmark 1: build/linux-x86_64-server-release/images/jdk/bin/java -XX:+UnlockExperimentalVMOptions -XX:+UseEpsilonGC -Xmx128m Hello
>>   Time (mean ± σ):      19.0 ms ±   0.3 ms    [User: 11.9 ms, System: 15.8 ms]
>>   Range (min … max):    18.2 ms …  20.8 ms    1000 runs
>> 
>> 
>> Additional testing:
>>  - [x] Linux AArch64 server fastdebug, `all`
>>  - [x] Linux x86_64 server fastdebug, `all`
>
> Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   David's review: more comments. Also reinstate has_PerfData in unprotected update.

> OK, how about this: add the blurb in the PerfData usage docs about the way to coordinate with shutdown if any other code ever updates the counters outside the safepoint, use that protection on the only path that @pchilano noted, leave this in PerfDataManager for any future use. See if we ever encounter a crash due to perf counter update elsewhere, promote its use it to "safe" pattern, if so. Once we feel sufficiently in clear, we remove the GC::write_synchronize part completely. I pushed this variant to PR now.
>
Sounds good to me. Thanks for the fix, I also like that we only add extra instructions for the problematic case in EnterI.

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

Marked as reviewed by pchilanomate (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23293#pullrequestreview-2586812327