Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Annotate JFR leak profiler with BasedOnJDKFile #10206

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

roberttoyonaga
Copy link
Collaborator

This is just a minor change that adds BasedOnJDKFile to the OldObjectSampler code.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Dec 2, 2024
@roberttoyonaga roberttoyonaga self-assigned this Dec 2, 2024
@christianhaeubl
Copy link
Member

Thanks.

Did you compare the current implementation with the HotSpot sources that you mentioned in the @BasedOnJDKFile annotations? I had a brief look and I think that some parts probably need to be updated on the GraalVM side (e.g., HotSpot seems to do some special handling for virtual threads, that seems to be missing on the GraalVM side).

@roberttoyonaga
Copy link
Collaborator Author

@christianhaeubl and @galderz

Thanks for catching that Christian. While looking into this, I actually discovered a bug that runs a lot deeper. What's happening in Hotspot is that the thread and stacktrace constant data are being cached and only written out when the OldObjectSample events get emitted. OldObjectSample events are special because they are only emitted at the end of a recording, not every chunk rotation. However, in SVM we are serializing the thread and stacktrace data normally like we do for any other event. This is bad because the constant data may get written out too early in chunk1 while the recording later finishes in chunk2. Thus separating the constant and event data. We probably didn't catch this before in tests because test recordings are generally only 1 chunk long.

The fix for this will likely not be trivial. I am happy to work on it but it might be best to do it in a separate PR. I can open a bug report for it too. What do you think?

@christianhaeubl
Copy link
Member

Very good catch, thanks for checking! Yes, please open a bug report so that this problem is documented.

It would be great if you could look into that. Regarding PRs, please do the fix and the @BasedOnJDKFile annotations in a single PR (code shouldn't have @BasedOnJDKFile annotations as long as it is out-of-sync with HotSpot).

@roberttoyonaga roberttoyonaga marked this pull request as draft December 5, 2024 14:14
@roberttoyonaga
Copy link
Collaborator Author

Very good catch, thanks for checking! Yes, please open a bug report so that this problem is documented.

It would be great if you could look into that. Regarding PRs, please do the fix and the @BasedOnJDKFile annotations in a single PR (code shouldn't have @BasedOnJDKFile annotations as long as it is out-of-sync with HotSpot).

Ok I'll add all the changes to this one PR. In the meantime, I've set this PR to "draft" and I'll open a bug report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
native-image native-image-jfr OCA Verified All contributors have signed the Oracle Contributor Agreement. redhat-interest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants