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

[Carter Kozak]

On Mon, 18 Dec 2023 19:33:19 GMT, Erik Gahlin <egahlin at openjdk.org> wrote:

> batches should at most be 25

That makes sense, I'd thought you had meant 10,000 events with unique periods :-)

> ```diff
> +    private void insertBatch(Batch batch) {
> +        for (Batch b : batches) {
> +            if (b.getPeriod() == batch.getPeriod()) {
> +                return; // Batch already exists
> +            }
> +        }
> +        batches.add(batch);
> +    }
> ```
I don't think it's correct to assume `b.getPeriod() == batch.getPeriod()` `b` is the same instance as `batch`, though. Consider the following scenario:

1. Event A is enabled at a 10 second interval
1. Event A is disabled (the batch is removed from the BatchManager)
1. Event B is enabled at a 10 second interval (this inserts a new Batch with the same 10 second period)
1. Event A is enabled again (still using the 10 second period).

In this scenario, Event A will have a Batch reference with the same period as the batch used by Event B, so `insertBatches` will return without changing the state of the BatchManager. In cases where the period duplicates an existing batch, I think we need register events to the existing batch.

Perhaps we should update `Batch findBatch(long period)` to `Batch findOrInsertBatch(long period, @Nullable Batch maybeCachedBatch)` where an existing registered batch is returned if one exists, otherwise `maybeCachedBatch` is inserted and returned if non-null, falling back to instantiating a new batch (the current behavior when an existing batch cannot be found). What do you think?

Sorry for slow responses today, I've been away from the computer -- I should have time to update this tomorrow.

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

PR Comment: https://git.openjdk.org/jdk/pull/17114#issuecomment-1861818082