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

[Erik Gahlin]

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 need to look more into this, and check things out manually, but what if BatchManager holds a SequencedSet of batches instead (LinkedHashSet).

When active sorted task are iterated and getTask() doesn't return null, the batch is added to batches field. If it already exists, nothing happens, otherwise it is inserted. 

I guess we could end up with two batches with same period, but maybe the period could be the key. A TreeMap.

> 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:
> 
> ```diff
> -                if (batch == null) {
> +                if (batch == null || !batches.contains(batch)) {
>                     batch = findBatch(task.getPeriod());
> ```

Someone might put 10 000 user-defined events in there.

The purpose of the reuse is not reduce allocation, but to make the period stable. Flipping back and forth should not reset things. I need to run with logging to see if it looks OK. Hard to simulate in the brain.

I don't think we have a complexity issue anymore.  I was thinkings tasks/events, but batches should at most be 25.

How would this work? If a batch with the same period exists, we reuse it, otherwise we insert it.


                     batch = findBatch(task.getPeriod());
+                } else {
+                    insertBatch(batch);
                 }
                 batch.add(task);
             }


 

+    private void insertBatch(Batch batch) {
+        for (Batch b : batches) {
+            if (b.getPeriod() == batch.getPeriod()) {
+                return; // Batch already exists
+            }
+        }
+        batches.add(batch);
+    }
+

Looks reasonable, but maybe this method signature would be easier to understand for someone that doesn't know the history?

    private Batch findBatch(long period, Batch oldBatch) {
      ...
    }

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

PR Comment: https://git.openjdk.org/jdk/pull/17114#issuecomment-1858136870
PR Comment: https://git.openjdk.org/jdk/pull/17114#issuecomment-1858181131
PR Comment: https://git.openjdk.org/jdk/pull/17114#issuecomment-1861424447
PR Comment: https://git.openjdk.org/jdk/pull/17114#issuecomment-1874469052