Release builds of libcomet skip LTO because the crate is also an rlib

[andygrove]

What is the problem the feature request solves?

native/Cargo.toml sets lto = "thin" for the release profile, but libcomet is never built with LTO. native/core declares crate-type = ["cdylib", "rlib"], where the rlib is for the criterion benchmarks, and Cargo only runs LTO for a unit when every one of its crate types supports it. An rlib does not, so Cargo drops the setting without a warning. The rustc line from cargo build --release -v -p datafusion-comet has no -C lto:

rustc --crate-name comet --edition=2021 core/src/lib.rs ... --crate-type cdylib --crate-type rlib --emit=dep-info,link -C opt-level=3 -C codegen-units=1 -C debuginfo=2 -C split-debuginfo=unpacked ... -C strip=debuginfo ...

rustc itself refuses -C lto with an rlib crate type ("lto can only be run for executables, cdylibs and static library outputs"). The release native libraries come from make core-amd64-libs and make core-arm64-libs, which run plain cargo build --release, so the published libcomet builds don't get LTO either.

The rlib also keeps symbols exported that a cdylib-only build would internalize. That is why each thread-local the allocation accounting wrapper touches costs its own __tls_get_addr call on x86_64 Linux: exported thread-locals use the general-dynamic TLS model, while internal ones share one call per function under local-dynamic. See the discussion on #6166.

Describe the potential solution

Measure it first. cargo rustc --release -p datafusion-comet --lib --crate-type cdylib overrides the crate types for that one build, and Cargo then passes -C lto=thin, so an LTO build of libcomet can be compared with the current one on TPC-H without changing the manifest. The build time matters too, since thin LTO runs over the whole dependency graph.

If LTO pays off, the release targets could build the library that way, or the cdylib could move into a small crate of its own that depends on datafusion-comet, leaving the rlib for the benchmarks.

Additional context

Found while reviewing #6166. A scratch crate with the same [lib] crate types and release profile gets the same flags from Cargo, and with --crate-type cdylib it gets -C lto=thin.

[andygrove]

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.

Per-query results

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.

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.

[andygrove]

@comphead @mbutrovich WDYT we should do?

[comphead]

IMO we can try different LTOs https://doc.rust-lang.org/cargo/reference/profiles.html#lto locally and check the benefits we got. I'm not sure unoptimized LTO is the bottleneck rn, and more LTO the longer compilation time. The release profile https://doc.rust-lang.org/cargo/reference/profiles.html#release carries thin local LTO and opt-level=3 and this might enough, but again we might want to run locally with fat LTO and see if it makes any difference

[mbutrovich]

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.

I think this is a good investigation, but I don't think LTO is the high pole in the tent from an optimization standpoint. I'd push for the third option just to make it clear what we're actually doing, and document why LTO isn't there with a link to this issue that it's not doing anything anyway.