-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[GR-60079] JFR leak profiler span fixes #10154
Conversation
Hi @galderz, just mentioning you so it's on your radar. |
Thanks for the fixes, this will be merged in the next few days. |
@roberttoyonaga : this PR causes the following assertion failure in some multi-threaded test cases:
Not directly related to this PR but I am also seeing assertion errors if I only run a subset of the tests, e.g., via
Can you look into these failures? |
Hi @christianhaeubl, yes ok I'll investigate what is happening there |
I've created PR fixing the issues you identified: #10190 The reason for the assertion failure is an incorrect assumption made by the assertion. The span fix in this PR causes the error to manifest, it was probably hidden before because span was allotted incorrectly. When running the |
Summary
Back when the original PR was integrated, we noted that there was probably a problem with how each sample's span was allotted, but we decided to match exactly what Hotspot was doing anyway. We were previously setting sample span to be the object allocation size. This is wrong because "span" is meant to represent all the allocations that span a period of time (even the ones that got removed from the queue due to GC). This is meant to have the effect of creating an even sampling representation of allocations over time, ex. if a sample's neighbors are removed from the list it should absorb their span and increase in importance so that its less likely to be removed itself.
Now that the fix is integrated in Hotspot (openjdk/jdk#19334), we should also add the fix in SubstrateVM.
I also fixed how
totalInQueue
is updated. Previously we weren't actually saving its updated size after adding samples to the queue.