mbutrovich commented on code in PR #3688:
URL: https://github.com/apache/datafusion-comet/pull/3688#discussion_r3066434327
##########
native/core/src/execution/jni_api.rs:
##########
@@ -527,23 +649,10 @@ pub unsafe extern "system" fn
Java_org_apache_comet_Native_executePlan(
// Retrieve the query
let exec_context = get_execution_context(exec_context);
- let tracing_event_name = match &exec_context.spark_plan.op_struct {
- Some(OpStruct::ShuffleWriter(_)) => "executePlan(ShuffleWriter)",
- _ => "executePlan",
- };
-
- if exec_context.tracing_enabled {
- #[cfg(feature = "jemalloc")]
- {
- let e = epoch::mib().unwrap();
- let allocated = stats::allocated::mib().unwrap();
- e.advance().unwrap();
- use crate::execution::tracing::log_memory_usage;
- log_memory_usage("jemalloc_allocated",
allocated.read().unwrap() as u64);
- }
- }
+ let tracing_event_name = exec_context.tracing_event_name.clone();
Review Comment:
This is doing a heap allocation and deep-copy of a `String` on a hot path
(per-batch) unconditionally. Can we guard this?
##########
native/core/src/execution/jni_api.rs:
##########
@@ -308,6 +410,21 @@ pub unsafe extern "system" fn
Java_org_apache_comet_Native_createPlan(
);
}
+ let session = Arc::new(session);
+
+ // Register this context's memory pool so we can sum all pools
+ // on the same thread when emitting tracing metrics.
+ let rust_thread_id = get_thread_id();
+ if tracing_enabled {
+ register_memory_pool(
+ rust_thread_id,
+ id,
+ Arc::clone(&session.runtime_env().memory_pool),
+ );
+ }
+
+ let tracing_event_name = build_tracing_event_name(&spark_plan);
Review Comment:
Do we need this if tracing isn't enabled? `create_plan` is less frequent
than `execute_plan`, but maybe still worth guarding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
