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

[ZGC][parser] Ensure that ZGCMemoryPoolSummary and OccupancySummary use kilobytes #227

Merged
merged 2 commits into from
Sep 6, 2022

Conversation

j-bahr
Copy link
Collaborator

@j-bahr j-bahr commented Sep 6, 2022

Ensure that ZGC memory values are normalized to kilobytes. The code in its current form will return a double without taking into account the units associated with the memory value. This can lead to multiple GC cycles that report odd results. This diff ensures that as they are parsed they are normalized to a long value in kilobytes. The regex's already capture the units so we'll simply scale the constructor inputs to ZGCMemoryPoolSummary and OccupancySummary by the captured unit string. We'll use the helper method .toKBytes(...) which is available to us in the the trace class already.

Please let me know if there is a project code formatting standard, I'd be happy to reformat to meet the project standards. While working, the editor cleaned up a number of spacing issues which lead to a slightly larger PR than expected. I wish this could have been picked up in a separate, format only diff. Anyways. Please let me know if I can better address this.

j-bahr and others added 2 commits September 5, 2022 19:50
…eceive values normalized to kilobytes. The code in its current form will return a double without taking into account the units associated with the memory. This can lead to multiple GC cycles that report odd results. This diff ensures that as they are parsed the are normalized to a long in terms of kilobytes
@j-bahr
Copy link
Collaborator Author

j-bahr commented Sep 6, 2022

Also, apologies for closing and re-opening this PR, I forgot to commit the test changes. They've been updated in this PR.

Copy link
Member

@karianna karianna left a comment

Choose a reason for hiding this comment

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

LGTM and previous PR was approved by Kirk so merging

@karianna karianna merged commit cec86bf into microsoft:main Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants