RFR: 8322142: JFR Periodic tasks aren't orphaned between recordings

[Carter Kozak]

On Thu, 14 Dec 2023 22:55:18 GMT, Carter Kozak <duke at openjdk.org> wrote:

> This issue is described on the jfr-dev mailing list here:
> https://mail.openjdk.org/pipermail/hotspot-jfr-dev/2023-December/005697.html
> With a minimal reproducer here:
> https://github.com/carterkozak/periodic-event-repro
> 
> Currently a draft because I don't have a ticket for this issue, and I haven't had a chance to add a test for this. I have verified that a build with this change resolves the bug I encountered.

I'm not certain what cases the BatchManager is optimized for, but I think the path forward depends quite a bit on that. array/ArrayList is difficult to outperform when inputs are small. TreeMap and LinkedHashSet both involve traversing many more pointers. I'm not sure how we want to weigh the benefit of reusing Batch objects between event disablement and reenablement against the higher traversal cost.

If we did move from ArrayList to a SequencedSet, instead of adding the orphaned flag or re-adding the task, we could update the existing null check along these lines:

-                if (batch == null) {
+                if (batch == null || !batches.contains(batch)) {
                    batch = findBatch(task.getPeriod());

Sounds good to me, I've applied your suggestion.

I'd appreciate if you could give me some guidance on adding a test for this fix. I may be able fit my reproducer (updated with a 1ms period) into the format of the tests in `jdk.jfr.api.flightrecorder`, but being a periodic test it will require some form of sleep/wait, and be fairly specific to this issue. Please let me know what you think the best path forward is, and I'll put something together :-)

Thanks for your help!

That works for me, thank you!

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

PR Comment: https://git.openjdk.org/jdk/pull/17114#issuecomment-1858168070
PR Comment: https://git.openjdk.org/jdk/pull/17114#issuecomment-1874507620
PR Comment: https://git.openjdk.org/jdk/pull/17114#issuecomment-1874527294