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.

Sign up for GitHub

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

Jump to bottom

[GR-48683] Add JFR event OldObjectSample. #8251

Merged
merged 5 commits into from
Jan 31, 2024
Merged

[GR-48683] Add JFR event OldObjectSample. #8251

merged 5 commits into from
Jan 31, 2024

Conversation

graalvmbot
Copy link
Collaborator

@graalvmbot graalvmbot commented Jan 25, 2024 •
edited by christianhaeubl

Based on #6743.

galderz and others added 2 commits November 17, 2023 09:53
@galderz
Add JFR old object event
d8a5916 
* Sample objects and arrays that are possible leaks.
* Sampling done in allocation slow path.
* It uses a custom priority queue ordered by span.
* The sampler uses a fixed-sized array at runtime
to keep the samples in memory.
By default this is 256 elements but it's configurable
via `-XX:FlightRecorderOptions`.
* Samples are scavenged when the queue is full.
* Checking if objects are garbage collected is done
using WeakReferences.
* Added JFR repository for serializing and emitting
old object information.
* Added plain object and array tests.
* Added tests that the sample queue to be full.
* Added old object description tests.
@christianhaeubl
Merge with master.
51201ad
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jan 25, 2024
@christianhaeubl christianhaeubl mentioned this pull request Jan 25, 2024
Closed
galderz
galderz approved these changes Jan 25, 2024
View reviewed changes
Copy link
Contributor

@galderz galderz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good. Thanks for the thorough refactoring and in particular thanks for showing how the testing backdoor can be used. There are not many examples of that in the oracle/graal source tree unfortunately.

@galderz
Copy link
Contributor

galderz commented Jan 25, 2024

There are some style issues that should be fixed.

@galderz
Copy link
Contributor

galderz commented Jan 25, 2024

One final thing @christianhaeubl, in case we want to backport this work to another GraalVM version, I was wondering if you're changes and my PR could be squashed and we put us both as co-authors?

@graalvmbot graalvmbot force-pushed the chaeubl/GR-48683 branch 2 times, most recently from f924a0d to fd2ee1a Compare January 26, 2024 07:48
@christianhaeubl
Copy link
Member

... in case we want to backport this work to another GraalVM version, I was wondering if you're changes and my PR could be squashed and we put us both as co-authors?

On our side, we prefer having individual commits.

I guess it would make backporting easier if we got rid of the merge commit that is between your and my commit. However, for that I would need to rebase this branch, which changes the hash of your commit and puts me as a co-author on your commit. As far as I know, this is not preferred by Red Hat.

@galderz
Copy link
Contributor

galderz commented Jan 26, 2024

Ok, let's leave it as is then.

@christianhaeubl
Copy link
Member

@roberttoyonaga, @galderz : I pushed one more commit where I did the following changes:

  • resolve the conflicts between this PR and [GR-51629] Add support for JFR -XX:FlightRecorderOptions. #8254 (FlightRecorderOptions)
  • changed the FlightRecorderOptions so that they match HotSpot on JDK21
    • removed globalbuffercount
    • renamed repositorypath to repository
    • renamed thread_buffer_size to threadbuffersize
  • improve the error reporting if something fails during JFR argument parsing

Please double-check and let me know if you find any issues.

@galderz
Copy link
Contributor

galderz commented Jan 29, 2024

All looks good, but I see CI still failing with style issues.

roberttoyonaga
roberttoyonaga reviewed Jan 29, 2024
View reviewed changes
substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jfr/JfrManager.java Outdated
Comment on lines 95 to 96
parseFlightRecorderLogging();
parseFlightRecorderOptions();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do have to do parse again here if we already did it in the previous hook?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I removed it.

@graalvmbot graalvmbot closed this Jan 31, 2024
@graalvmbot graalvmbot merged commit 6fdaeca into master Jan 31, 2024
12 checks passed
@graalvmbot graalvmbot deleted the chaeubl/GR-48683 branch January 31, 2024 00:40
@galderz
Copy link
Contributor

galderz commented Jan 31, 2024

Thanks @christianhaeubl!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christianhaeubl christianhaeubl christianhaeubl left review comments

@roberttoyonaga roberttoyonaga roberttoyonaga approved these changes

@galderz galderz galderz approved these changes

Assignees

@christianhaeubl christianhaeubl

Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@graalvmbot @galderz @christianhaeubl @roberttoyonaga