[GR-42740] [GR-44248] Experimental support for JFR event streaming.

[graalvmbot]

No description provided.

[christianhaeubl]

@roberttoyonaga : this is basically your PR with a bunch of changes on top because I ran into larger issues:

• Locking now happens on the JfrBufferNode and no longer on the JfrBuffer.
• Added a retired flag because Java-level JfrBuffers can't be freed right away.
• Disabled flushing of stack traces because it would break the stack trace data.
• Fixed and improved the test cases.
• Fixed various other concurrency-related issues.
• Fixed various issues related to memory management.
• Various refactorings, cleanups, and more documentation.

There are still a few known issues:

• If flushing is disabled, then we leak JfrBufferNodes (those nodes are only cleaned up when JFR recording stops).
• When flushing the JfrRepository data, it can happen that repositories don't contain any unflushed data. In that case, the JfrChunkWriter writes some unnecessary information (i.e., that the repository was empty).
• destroyJfr() still needs some improvements.

Feel free to review the changes.

[roberttoyonaga]

I think this comment might not be true anymore. We flush all the local buffers to the global buffers, but the java local buffers do not get freed here only retired.

[roberttoyonaga]

Is this assertion and the Javadoc above always true? When destroyJfr is called and the java buffer node list is being torn down, when are the JfrBuffers freed before reaching here?

[christianhaeubl]

You are right, this is one of the open topics. I need to think about this a bit more because I don't have a good solution yet.

[roberttoyonaga]

Hi Christian, thank you for resolving those issues. The new changes seem reasonable to me. I only have a few questions.

• If flushing is disabled, then we leak JfrBufferNodes (those nodes are only cleaned up when JFR recording stops

But even if there is no flushing, won't the node lists still be traversed during an epoch change, and old nodes cleaned up?

[christianhaeubl]

But even if there is no flushing, won't the node lists still be traversed during an epoch change, and old nodes cleaned up?

That is right, I forgot to mention that. The problem is that a lot of time can pass before an epoch change happens (i.e., a lot of threads could start and die in that time). I think this is something that we should address at some point but it isn't super important because it most likely won't cause any issues in practice.

[roberttoyonaga]

a lot of threads could start and die in that time

This is what I was a bit concerned about before, and why I had originally had an eager cleanup scheme where nodes attempt to be removed once their threads die. Do you think we should try to re-implement something similar to that at some time in the future?

[roberttoyonaga]

Would it be better to check with compareFoundAndExpectedIds() after each constant pool? This way we can validate each flush individually to make sure there's no unexpected references at any intermediate point?

[christianhaeubl]

Good point, we can indeed verify the accumulated constant pool data after every checkpoint. I changed this so that the code is now similar to what you had in place before.

[christianhaeubl]

This is what I was a bit concerned about before, and why I had originally had an eager cleanup scheme where nodes attempt to be removed once their threads die. Do you think we should try to re-implement something similar to that at some time in the future?

Yes, but I think that we can use something simple, like an explicit lock for iterating/removing nodes (instead of relying on the JfrChunkWriter lock).