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

Move g1 heapRegionSize from class variable to member variable #324

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

loyispa
Copy link
Contributor

@loyispa loyispa commented Jan 25, 2024

Concurrent GCToolKit instances with different g1 region-size will affect each other because of their static sharing of G1GCForwardReference#heapRegionSize. Consider moving it to a member variable.

@karianna karianna requested review from kcpeppe and dsgrieve January 26, 2024 01:57
@karianna
Copy link
Member

@loyispa Interesting, I'd love to learn more about how/why you're running multiple instances of GCToolkit if you're able to share?

@loyispa
Copy link
Contributor Author

loyispa commented Jan 26, 2024

@karianna, I am developing a tool that can analyze multiple streaming logs using this great library. I am currently facing a problem if multiple logs of G1 with different region sizes were analyzed concurrently, there would be incorrect calculations for Eden and Survival occupancy because of the sharing of heapRegionSize.

@kcpeppe
Copy link
Collaborator

kcpeppe commented Jan 26, 2024

IMHO, I don't think it's correct to include region size with each G1 event. However, there isn't an alternative context to put this static/never changing for a single GC log. IOWs, we didn't give too much thought to parsing multiple logs at the same time. I was thinking that maybe adding a session object might help but then I was thinking that there are other problems you'd run into for this use case. I'm inclined to merge this with the caveat that it might be deprecated in favour of a better solution. @dsgrieve would you care to offer an opinion here?

@dsgrieve
Copy link
Member

IMHO, I don't think it's correct to include region size with each G1 event. However, there isn't an alternative context to put this static/never changing for a single GC log. IOWs, we didn't give too much thought to parsing multiple logs at the same time. I was thinking that maybe adding a session object might help but then I was thinking that there are other problems you'd run into for this use case. I'm inclined to merge this with the caveat that it might be deprecated in favour of a better solution. @dsgrieve would you care to offer an opinion here?

@kcpeppe I'm inclined to accept the change with some minor modifications. This will allow @loyispa to move forward but doesn't prevent a possible alternative in the future.

@loyispa loyispa requested a review from dsgrieve January 29, 2024 05:58
@karianna karianna merged commit 3cb229b into microsoft:main Jan 30, 2024
@loyispa loyispa deleted the fix-g1-parser branch January 30, 2024 05:25
@karianna karianna mentioned this pull request Feb 9, 2024
@karianna karianna mentioned this pull request Mar 28, 2024
@karianna karianna mentioned this pull request May 9, 2024
@karianna karianna mentioned this pull request Jul 3, 2024
@karianna karianna mentioned this pull request Oct 1, 2024
@karianna karianna mentioned this pull request Dec 8, 2024
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.

4 participants