perf: Add memory profiling #1702
Conversation
|
@mbutrovich I'd like to get your feedback on this PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1702 +/- ##
============================================
+ Coverage 56.12% 58.69% +2.56%
- Complexity 976 1143 +167
============================================
Files 119 129 +10
Lines 11743 12615 +872
Branches 2251 2359 +108
============================================
+ Hits 6591 7404 +813
- Misses 4012 4038 +26
- Partials 1140 1173 +33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -359,6 +365,21 @@ pub unsafe extern "system" fn Java_org_apache_comet_Native_executePlan( | |||
| // Retrieve the query | |||
| let exec_context = get_execution_context(exec_context); | |||
|
|
|||
| // memory profiling is only available on linux | |||
| if exec_context.memory_profiling_enabled { | |||
| #[cfg(target_os = "linux")] | |||
There was a problem hiding this comment.
should the if statement be inside cfg?
There was a problem hiding this comment.
I did that originally, but the compiler complained about the unused variable memory_profiling_enabled on non-Linux platforms.
| // scalastyle:off println | ||
| println( | ||
| "JVM_MEMORY: { " + | ||
| s"heapUsed: ${mb(heap.getUsed)}, heapCommitted: ${mb(heap.getCommitted)}, " + | ||
| s"nonHeapUsed: ${mb(nonHeap.getUsed)}, nonHeapCommitted: ${mb(nonHeap.getCommitted)} " + | ||
| "}") |
There was a problem hiding this comment.
Perhaps logInfo ?
If println is preferred, we may have to add back // scalastyle:on println
|
Moving to draft while I experiment with jemalloc-specific logging |
|
I added some @mbutrovich does this seem useful? |
|
I'm not sure how to reconcile the different resident numbers: edit: updated to use consistent MB formatting edit: resident memory is close to heapUsed + jemalloc resident, but is this the correct interpretation? |
| println!( | ||
| "NATIVE_MEMORY: {{ resident: {:.0} }}", | ||
| (statm.resident * page_size) as f64 / (1024.0 * 1024.0) | ||
| ); |
There was a problem hiding this comment.
| println!( | |
| "NATIVE_MEMORY: {{ resident: {:.0} }}", | |
| (statm.resident * page_size) as f64 / (1024.0 * 1024.0) | |
| ); |
| // Read statistics using MIB key: | ||
| let allocated = allocated.read().unwrap() as f64 / (1024.0 * 1024.0); | ||
| let resident = resident.read().unwrap() as f64 / (1024.0 * 1024.0); | ||
| println!( |
There was a problem hiding this comment.
wondering should we feed the metrics to log or to plan metrics
There was a problem hiding this comment.
These metrics are for the overall process, so not specific to any particular plan. This feature just allows us to watch the executor logs and see when memory starts to approach the limit. I hope this will help us debug the cause of OOMs, although it will still primarily be a manual process.
|
Thanks for the reviews @comphead @kazuyukitanimura and @mbutrovich (offline review). I'm going to go ahead and merge this and then iterate on improving it once I have used it to debug a real issue. |
|
Not the end of the world, but this introduces warnings on macOS when using jemalloc: |
Which issue does this PR close?
Closes #1701
Rationale for this change
We need a way to profile memory.
What changes are included in this PR?
Logging of JVM and native memory utilization on each call to
execute_plan. After running TPC-H locally with 8GB executor memory and 8GB off-heap memory, I see these stats:When running with multiple iterations, I see overall memory exceeding 16GB. If this were running in a container environment then I would expect to see the executor be killed due to OOM.
How are these changes tested?