-
Notifications
You must be signed in to change notification settings - Fork 7
Potential memory leak with the JVMTI wallclock sampler #234
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
base: main
Are you sure you want to change the base?
Conversation
🔧 Report generated by pr-comment-scanbuild |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
@@ -165,6 +165,7 @@ class WallClockJVMTI : public BaseWallClock { | |||
struct ThreadEntry { | |||
ddprof::VMThread* native; | |||
jthread java; | |||
int tid; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit - maybe premature optimization but can we 'unionize' the native
and tid
since they will never be used at the same time?
What does this PR do?:
Release
jthread
local reference to prevent memory leak.Profiler uses
jvmtiError GetAllThreads(jvmtiEnv* env, jint* threads_count_ptr, jthread** threads_ptr)
to obtain a list of running threads. The document statesThe returned array contains JNI local references of threads, should be managed by caller, which means the caller should manage the life cycle of returned JNI local reference. In this case, we should delete those JNI local references to avoid the leak.
Motivation:
Make JVMTI wallclock sampler useable.
Additional Notes:
How to test the change?:
Run:
It crashes without this fix, no crash with the fix.
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!