andygrove commented on issue #6210: URL: https://github.com/apache/datafusion-comet/issues/6210#issuecomment-5836944939
Findings from trying this out. A decision is needed on which approach to take; the options are at the end. **Making LTO apply.** Moving the cdylib into a crate of its own works. A new `native/cdylib` crate (`datafusion-comet-cdylib`, `[lib] name = "comet"`, `crate-type = ["cdylib"]`) holds nothing but `extern crate datafusion_comet;`, and `native/core` becomes a plain rlib named `datafusion_comet`. Cargo then passes `-C lto=thin` to the cdylib and builds core and every dependency with `-C linker-plugin-lto`. The `extern crate` line is required: rustc does not link a dependency that the crate never refers to, and without it the library exports none of the `Java_*` functions. `libcomet` keeps its name and path and exports the same 49 dynamic symbols (23 `Java_*`) as today. The change passes clippy, `cargo machete`, `dev/verify-contrib-delta-gate.sh` (updated to build the new package) and `CometNativeSuite`. **Build cost.** Main against that change on a 32-core x86_64 Linux machine (rustc 1.95.0, default features, `RUSTFLAGS=-Ctarget-cpu=x86-64-v3` as in `make core-amd64-libs`), each in a clean target directory: | | main (no LTO) | thin LTO | | --- | --- | --- | | Clean `cargo build --release`, wall / user CPU | 6:10 / 2,054 s | 6:36 / 2,404 s | | Rebuild after touching one core source file, wall / user CPU | 1:21 / 77 s | 3:04 / 1,040 s | | Peak memory of the largest process | 7.7 GiB | 14.5 GiB | | `libcomet.so` size | 133 MB | 156 MB | The rebuild row is what `make release` costs on every change, because thin LTO reruns over the whole dependency graph. Memory was measured with 32 jobs. The release container (`CARGO_BUILD_JOBS=2`, 24 GB) and the snapshot runners (16 GB) run fewer LTO threads, which should need less memory and take longer; that was not measured. **Runtime.** TPC-H SF100 on the same machine (Spark 4.1.1 standalone, 2 executors x 8 cores, 16 GB off-heap), 3 rounds of both builds alternating which ran first, 3 iterations per run. Times are the median of iterations 2 and 3 across the rounds, so 6 samples per query per build: | | main (no LTO) | thin LTO | change | | --- | --- | --- | --- | | Total, 22 queries | 197.3 s | 196.6 s | -0.4% | | Q21 | 30.66 s | 29.53 s | -3.7% | | Q4 | 4.44 s | 4.53 s | +2.0% | Q21 and Q4 are the only queries whose ranges do not overlap. The other 20 move by at most 3.6%, with overlapping ranges. Every run of every query returned the same result hash. <details> <summary>Per-query results</summary> | Query | main (s) | thin LTO (s) | change | main range (s) | thin LTO range (s) | | ---: | ---: | ---: | ---: | --- | --- | | 1 | 4.13 | 4.20 | +1.6% | 4.07 to 4.25 | 4.17 to 4.24 | | 2 | 4.15 | 4.11 | -1.0% | 4.13 to 4.28 | 4.07 to 4.18 | | 3 | 9.13 | 9.13 | +0.0% | 9.05 to 9.36 | 9.06 to 9.17 | | 4 | 4.44 | 4.53 | +2.0% | 4.39 to 4.48 | 4.49 to 4.55 | | 5 | 17.16 | 17.13 | -0.2% | 17.05 to 17.29 | 17.08 to 17.31 | | 6 | 1.00 | 1.03 | +2.7% | 0.98 to 1.03 | 1.01 to 1.06 | | 7 | 8.42 | 8.50 | +1.0% | 8.35 to 8.50 | 8.45 to 8.53 | | 8 | 15.03 | 15.00 | -0.2% | 14.80 to 15.12 | 14.89 to 15.14 | | 9 | 26.20 | 26.46 | +1.0% | 26.03 to 26.81 | 26.11 to 26.57 | | 10 | 7.58 | 7.55 | -0.3% | 7.45 to 15.49 | 7.52 to 10.04 | | 11 | 4.44 | 4.43 | -0.2% | 4.35 to 4.62 | 4.41 to 4.49 | | 12 | 4.20 | 4.15 | -1.1% | 4.01 to 4.54 | 4.05 to 4.20 | | 13 | 3.95 | 4.09 | +3.6% | 3.81 to 5.73 | 3.87 to 5.70 | | 14 | 2.16 | 2.11 | -2.0% | 2.09 to 2.20 | 2.08 to 2.29 | | 15 | 4.41 | 4.33 | -1.8% | 4.23 to 4.53 | 4.29 to 4.61 | | 16 | 2.35 | 2.33 | -0.7% | 2.31 to 2.44 | 2.31 to 2.37 | | 17 | 20.35 | 20.40 | +0.3% | 20.21 to 20.70 | 20.30 to 20.79 | | 18 | 18.87 | 18.92 | +0.3% | 18.71 to 19.00 | 18.64 to 18.99 | | 19 | 2.65 | 2.62 | -1.2% | 2.61 to 2.69 | 2.60 to 2.71 | | 20 | 3.93 | 3.91 | -0.5% | 3.87 to 3.96 | 3.87 to 3.95 | | 21 | 30.66 | 29.53 | -3.7% | 30.30 to 30.75 | 29.35 to 29.65 | | 22 | 2.14 | 2.15 | +0.4% | 2.11 to 2.18 | 2.12 to 2.24 | | Total | 197.32 | 196.62 | -0.4% | | | Q10 is bimodal in both builds (about 7.5 s, or 10 to 15 s), so its median depends on how the runs fall. </details> **Where the Q21 gain comes from.** Not the TLS model. Since #6166, the allocator shims (`__rust_alloc`, `__rust_dealloc`, `__rust_realloc`, `__rust_alloc_zeroed`) make one `__tls_get_addr` call each in both builds, and both libraries carry the same 18 `R_X86_64_DTPMOD64` relocations. What changes is inlining. Under LTO the accounting allocator and the `malloc`/`free` calls behind it are inlined into callers across the library: direct `malloc` call sites go from 1,550 to 51,242, `free` from 5,335 to 79,044, and `__tls_get_addr` from 8,005 to 111,474. That saves a call on every allocation. Q21 is also the query that was most sensitive to the allocator wrapper's cost in #6166. **Decision needed.** The options: 1. **Split the crate, so every release build gets LTO.** The published libraries and `make release` both get the numbers above. The cost is the build table: every release rebuild takes about 13x the CPU, and peak build memory roughly doubles. 2. **LTO for the published libraries only.** Build the release targets (`core-amd64-libs`, `core-arm64-libs`) with `cargo rustc --release -p datafusion-comet --lib --crate-type cdylib` and leave everything else alone. The shipped library would be the same LTO build (not measured separately) without the local rebuild cost. But `make release`, which the benchmark docs use, would no longer build the library that users get, and other `cargo build --release` paths would keep skipping LTO without a warning. 3. **Remove `lto = "thin"` from the release profile.** The profile then says what actually gets built, and build times stay as they are, giving up the Q21 gain. -- 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]
