sunchao commented on code in PR #5934:
URL: https://github.com/apache/datafusion-comet/pull/5934#discussion_r4012178025
##########
native/core/src/lib.rs:
##########
@@ -65,28 +65,66 @@ pub mod jvm_bridge {
use errors::{try_unwrap_or_throw, CometError, CometResult};
+pub mod alloc_accounting;
pub mod cloud;
pub mod execution;
pub mod parquet;
// this module is for non release only. Intended for debugging/profiling
purposes
#[cfg(debug_assertions)]
pub mod debug;
+// The global allocator is the selected backend (jemalloc, mimalloc, or the
system allocator),
+// optionally wrapped in `AccountingAllocator` when the `alloc-accounting`
feature is on. The cfgs
+// below are mutually exclusive so exactly one `#[global_allocator]` is
defined; a build without
+// the feature is byte-for-byte the previous arrangement, with no wrapper and
no per-allocation
+// work.
+
#[cfg(all(
not(target_env = "msvc"),
feature = "jemalloc",
- not(feature = "mimalloc")
+ not(feature = "mimalloc"),
+ not(feature = "alloc-accounting")
))]
#[global_allocator]
static GLOBAL: Jemalloc = Jemalloc;
#[cfg(all(
feature = "mimalloc",
- not(all(not(target_env = "msvc"), feature = "jemalloc"))
+ not(all(not(target_env = "msvc"), feature = "jemalloc")),
+ not(feature = "alloc-accounting")
))]
#[global_allocator]
static GLOBAL: MiMalloc = MiMalloc;
+#[cfg(all(
+ not(target_env = "msvc"),
+ feature = "jemalloc",
+ not(feature = "mimalloc"),
+ feature = "alloc-accounting"
+))]
+#[global_allocator]
+static GLOBAL: alloc_accounting::AccountingAllocator<Jemalloc> =
+ alloc_accounting::AccountingAllocator::new(Jemalloc);
+
+#[cfg(all(
+ feature = "mimalloc",
+ not(all(not(target_env = "msvc"), feature = "jemalloc")),
+ feature = "alloc-accounting"
+))]
+#[global_allocator]
+static GLOBAL: alloc_accounting::AccountingAllocator<MiMalloc> =
+ alloc_accounting::AccountingAllocator::new(MiMalloc);
+
+// Accounting over the system allocator: neither mimalloc nor a usable
jemalloc.
+#[cfg(all(
+ feature = "alloc-accounting",
+ not(feature = "mimalloc"),
+ any(target_env = "msvc", not(feature = "jemalloc"))
Review Comment:
### Correctness
[P2] Could the system fallback also cover the case where both allocator
features are enabled, or reject that combination explicitly? On non-MSVC
targets, `jemalloc,mimalloc,alloc-accounting` makes every `GLOBAL` definition
false: each backend excludes the other, and this fallback excludes `mimalloc`.
The process therefore uses an unwrapped default allocator while
`log_native_allocated` is still enabled, so the new metric silently stays zero.
I reproduced the selection with this exact cfg block and the exact accounting
module: holding an 8 MiB buffer moved the balance with either individual
backend configuration, but left it at zero with both features. The probe
substitutes `System` for the backend types, so it tests allocator selection
rather than jemalloc/mimalloc behavior. A combination check would prevent
publishing a plausible zero metric when accounting was requested.
--
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]
