sunchao commented on code in PR #5934: URL: https://github.com/apache/datafusion-comet/pull/5934#discussion_r4017037738
########## native/core/benches/alloc_overhead.rs: ########## @@ -0,0 +1,202 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Measures the cost the `alloc-accounting` global-allocator wrapper adds per allocation. +//! +//! Run the same benchmark with and without the feature and compare: +//! +//! ```shell +//! cargo bench --bench alloc_overhead -- --save-baseline off +//! cargo bench --bench alloc_overhead --features alloc-accounting -- --baseline off +//! ``` +//! +//! The benchmark relies on the `#[global_allocator]` that `lib.rs` installs, which reaches this +//! binary through the `rlib`. That only happens if the crate is actually linked, and an `--extern` +//! crate that nothing names is dropped from the crate graph along with its allocator, so the +//! `extern crate` below is load-bearing: without it a baseline run that never touches `comet` +//! silently measures the system allocator instead of jemalloc. The two liveness checks fail the run +//! if either the selected backend or the wrapper is somehow not in effect, because a number +//! measured against the wrong allocator would be worse than no number. +//! +//! `small_churn` is the worst case for the thread-local path: allocations so small that the +//! wrapper's bookkeeping is a meaningful fraction of the allocator's own work. `threshold_churn` is +//! the worst case for the shared counter: an alloc/free loop at exactly the 64 KiB settle threshold +//! flushes to the process-wide atomic on every call, and the parallel variant does that from every +//! core at once, so the gap between the single-threaded and parallel numbers is the cost of +//! contention on that cacheline. `arrow_sized_churn` is closer to what Comet actually does, where +//! a batch-sized buffer dwarfs the bookkeeping. Real query workloads sit at or below +//! `arrow_sized_churn`, because they do actual work between allocations. + +// Pulls `comet`, and with it the `#[global_allocator]` selected by its feature set, into this +// binary even when the feature set leaves nothing here that names the crate. +extern crate comet; + +use criterion::{criterion_group, criterion_main, BatchSize, Criterion, Throughput}; +use std::hint::black_box; +use std::thread; +use std::time::Instant; + +/// Guards against measuring the wrong allocator. jemalloc keeps its own count of bytes it has +/// served; if it is not the global allocator of this binary that count stays at zero, and a +/// "jemalloc" baseline would in fact be the system allocator. +#[cfg(feature = "jemalloc")] Review Comment: [P2] Match the liveness guard to the selected allocator ### Correctness Could this guard use the same condition as the jemalloc backend in `lib.rs`? On non-MSVC targets, `--features jemalloc,mimalloc` deliberately selects the system allocator, but this condition still enables the jemalloc assertion. The held 8 MiB allocation therefore goes through `System`, while the guard checks jemalloc's separate allocation statistics. The benchmark aborts before collecting measurements for that accepted combination, both with and without `alloc-accounting`. Restricting this guard to `all(not(target_env = "msvc"), feature = "jemalloc", not(feature = "mimalloc"))`, with the no-op using its complement, keeps the linkage check for actual jemalloc builds and permits the mixed-feature system baseline. -- 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]
