andygrove opened a new issue, #6165: URL: https://github.com/apache/datafusion-comet/issues/6165
### Describe the enhancement With the allocation accounting wrapper always installed (#6162), TPC-H SF100 Q21 runs about 5% slower. It was slower in every run on every build tested: median 29.7 s vs 31.2 s, and 30.4 s vs 32.1 s in a separate Q21-only A/B. The rest of the suite is within noise, and the total is +0.8 to 1.1%. This was measured on a 32-core Linux box with Spark 4.1.1, 2 executors x 8 cores, and the system allocator (glibc, no jemalloc). A `perf` profile of Q21 (3 iterations, both executors, about 3.2M samples per build) attributes the difference to the wrapper's thread-local access: | Where the time goes | Base | With wrapper | | --- | --- | --- | | `__rust_alloc` + `__rust_dealloc` self time (the wrapper is inlined here) | 0.48% | 2.48% | | `__tls_get_addr` (the dynamic loader's thread-local lookup, plus its `@plt` stub) | 0.06% | 2.40% | | glibc `malloc` / `free` / `_int_free` | 8.5% | 7.2% | That is about 4.3 points of extra samples, which roughly matches the slowdown. `libcomet.so` is `dlopen`ed by the JVM, so its `thread_local!` variables use the general-dynamic TLS model, and every access calls `__tls_get_addr`. `alloc_accounting::track` makes three thread-local accesses on every allocation and every free: - `IN_TRACK.replace(true)` - `LOCAL_DRIFT.try_with(...)`, which also checks the lazily registered destructor's state - `IN_TRACK.set(false)` The `alloc_overhead` benchmark does not show this. It links `comet` as an rlib into an executable, where thread-local access is a single instruction, so only the production shared library pays for the lookup. ### Proposed fix Make one thread-local access per call. Keep the re-entrancy flag and the un-flushed per-thread delta in a single `const`-initialized thread-local with no destructor, so the fast path is one `__tls_get_addr`, a flag check and an add, with no lazy-initialization state check. Settling the remaining delta at thread exit still needs a destructor. That can move to a separate guard thread-local that is touched once per thread, the first time the thread allocates, with the re-entrancy flag set, because registering that destructor through `__cxa_thread_atexit_impl` allocates. If that is not enough, the next option is to avoid thread-locals altogether, for example with counters sharded by stack address, which trades the lookup for occasional contention. ### Validation - Add an `alloc_overhead` case that reproduces the shared-library TLS cost, for example by benchmarking through a `cdylib` loaded with `dlopen`. - Rerun the TPC-H SF100 Q21 A/B and `perf` profile against the current 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]
