andygrove commented on code in PR #5934: URL: https://github.com/apache/datafusion-comet/pull/5934#discussion_r4017097065
########## 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: Confirmed and fixed in ac51c5c. With `jemalloc,mimalloc` the library selects the system allocator but the bench still asserted on jemalloc's counters, so that combination aborted before measuring anything. Rather than mirror the three-term predicate in the bench, `lib.rs` now exports the selection it made: each `backend` module declares a `NAME` and the crate re-exports it as `comet::ALLOCATOR_BACKEND` (`"jemalloc"`, `"mimalloc"` or `"system"`). The bench announces that name at startup and runs the jemalloc liveness assertion only when the library reports jemalloc. The selection is still written once, so the guard cannot drift from it the way a copied cfg could, which is how the previous P2 crept in. Verified with clippy on the bench target for all eight feature combinations, and a criterion `--test` run of `alloc_overhead` under `jemalloc,mimalloc` and `jemalloc,mimalloc,alloc-accounting`, both of which now report `measuring the `system` allocator backend` and complete; `jemalloc` alone still runs the assertion and passes. -- 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]
