perf: Add performance tracing capability #1706
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1706 +/- ##
============================================
+ Coverage 56.12% 58.69% +2.56%
- Complexity 976 1142 +166
============================================
Files 119 129 +10
Lines 11743 12637 +894
Branches 2251 2363 +112
============================================
+ Hits 6591 7417 +826
- Misses 4012 4044 +32
- Partials 1140 1176 +36 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
andygrove
changed the title
andygrove
changed the title
There was a problem hiding this comment.
Thanks @andygrove btw did you try the samply
https://datafusion.apache.org/library-user-guide/profiling.html#profiling-using-samply-cross-platform-profiler
I experimented with it a little bit and documented in DF
UPD: it works with binaries profiling, but I'm not sure how to use it in hybrid env like Comet through
I was looking for a solution where we can choose which sections of code to trace, both in JVM and native, and see the flow between these layers. There are |
native/core/src/execution/jni_api.rs
Outdated
| @@ -767,3 +750,20 @@ pub unsafe extern "system" fn Java_org_apache_comet_Native_traceEnd( | |||
| ) { | |||
| // no implementation | |||
| } | |||
|
|
|||
| struct TraceGuard<'a> { | |||
There was a problem hiding this comment.
perhaps we can have it as generic function?
pub fn trace_time<T, F>(label: &str, mut f: F) -> T
where
F: FnMut() -> T,
{
let _ = TraceGuard::new("writeSortedFileNative");
let result = f();
result
}
There was a problem hiding this comment.
Yes, I like that idea. I will try and implement this.
There was a problem hiding this comment.
I ran into some issues around lifetimes for captured variables with this approach, so perhaps it is simpler to stick with the TraceGuard.
|
We can trace native memory usage as well and display it next to the running tasks:
|
|
@Kontinuation you may be interested in this |
| } | ||
|
|
||
| fn log_event(&self, name: &str, ph: &str) { | ||
| println!( |
There was a problem hiding this comment.
is it planned to output with println or some other method like tracing https://docs.rs/tracing/latest/tracing/ ?
There was a problem hiding this comment.
I created an epic where we can track items like this.
There was a problem hiding this comment.
Yes it would be nicer to use tracing (and even better to have the tracing output compatible with log like this perhaps? https://docs.rs/tracing/latest/tracing/#for-log-users ).
| @@ -133,29 +133,25 @@ class CometExecIterator( | |||
|
|
|||
| def getNextBatch(): Option[ColumnarBatch] = { | |||
| assert(partitionIndex >= 0 && partitionIndex < numParts) | |||
| try { | |||
| nativeLib.traceBegin("CometExecIterator_getNextBatch") | |||
There was a problem hiding this comment.
should it be inside if (memoryProfilingEnabled ?
|
I moved this to draft because it is still quite experimental. I am now working on adding tracing for JVM off-heap usage in |
|
I can now see the off-heap memory being used by
|
| } | ||
|
|
||
| fn log_event(&self, name: &str, ph: &str) { | ||
| println!( |
There was a problem hiding this comment.
Yes it would be nicer to use tracing (and even better to have the tracing output compatible with log like this perhaps? https://docs.rs/tracing/latest/tracing/#for-log-users ).
| self.log_event(name, "E") | ||
| } | ||
|
|
||
| pub fn log_counter(&self, name: &str, value: usize) { |
There was a problem hiding this comment.
Can this be u64 and so not be system dependent? Not sure how useful it might be but some counters (say bytes read) could be u64 but usize may be smaller.
Which issue does this PR close?
Closes #1705
Rationale for this change
This feature makes it possible to visualize the flow of calls during query execution, along with JVM and native memory usage.
What changes are included in this PR?
Adds optional feature to enable trace logging using Chrome's Trace Event Format.
How are these changes tested?
I tested manually and confirmed that there is no performance impact when this feature is not enabled.