andygrove commented on code in PR #5934:
URL: https://github.com/apache/datafusion-comet/pull/5934#discussion_r4016507834
##########
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:
Confirmed and fixed in 2d78f87. On main, `jemalloc,mimalloc` on a non-MSVC
target already selects neither arm and falls through to the system allocator;
the accounting fallback here excluded `mimalloc`, so that combination with
`alloc-accounting` matched nothing.
The selection is now a `backend` module chosen by three cfgs that partition
every feature combination: jemalloc where it builds and mimalloc was not
requested, mimalloc otherwise, and the system allocator as the exact complement
of those two (which is where `jemalloc,mimalloc` still lands, as on main). The
unwrapped `#[global_allocator]` lives inside the jemalloc and mimalloc modules,
so a build without the feature is unchanged and still installs nothing for the
system case. The single accounting `#[global_allocator]` refers to
`backend::Backend`, so a combination with no backend is a compile error rather
than a silent zero.
Verified with `cargo check` on all eight combinations of the three features,
plus `cargo test --features jemalloc,mimalloc,alloc-accounting
alloc_accounting`, where `a_real_allocation_raises_the_balance` (the test that
checks the wrapper is really installed for the current feature set) passes.
While doing that I found the two feature-gated tests were flaky under the
feature (2 of 20 runs): `BALANCE` is process-wide and the rest of the crate's
tests, plus the 64 MiB block in `dealloc_settles_before_delegating`, move it
concurrently. The same commit makes them noise-proof, and the thread-exit test
no longer needs the feature, so it now runs in the default CI build.
--
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]
