dwsmith1983 commented on PR #5612: URL: https://github.com/apache/datafusion-comet/pull/5612#issuecomment-5511332599
Thanks for pushing on all three points. You were right on both technical claims, so taking them in order. Harness and raw data: https://gist.github.com/dwsmith1983/e46e22c1c594b4f5120515c773f2b3ef has the full harness source, exact build and run commands, both commit SHAs, toolchain and dependency versions, and per-cell CSVs for both replicates including the regressing shared-instance cells. Sort path: your reading of ExternalSorter checks out and the repro confirms it. With the exact plan shape Comet produces (SortExec, no fetch, single partition, 128 x 8192-row batches so the reservation is well past sort_in_place_threshold_bytes), a tracking shim around one UDF instance measured max 9 concurrent in-flight evaluations at 8 runtime workers, and even 2 at 1 worker since the merge evaluates concurrently with a spawned sort task. On that path regexp_extract_all as the sort key was 1.45x slower than base at 1 worker and 2.2x at 8. regexp_extract as the key was parity to slightly faster. Attribution: you were right that my scratch-pool explanation was wrong. Clone creates a fresh private pool (meta/regex.rs 1916-1926), so scratch state is never shared. The real mechanism, isolated in a micro benchmark in the gist, is per-row refcount traffic: captures_iter creates a Captures per row via create_captures, which is Captures::all(self.group_info().clone()), an Arc clone against the program-owned GroupInfo, plus one more Captures clone per match. With every thread holding clones of one compiled program, that single refcount cache line bounces across cores and caps throughput regardless of thread count. A variant with one clone per thread, no lock and no per-invoke clone still collapses identically, which rules out the mutex and the clone itself. regexp_extract is immune because it reuses one CaptureLocations across rows, and split never creates a Captures. That pointed at the fix, now pushed: regexp_extract_all drives iteration with find_iter (identical span semantics, verified against the crate's shared iterator code and pinned with empty-match and multibyte edge tests) and resolves groups through captures_read_at into one CaptureLocations reused per batch, same as regexp_extract. Rerun results: the sort scenario goes from 2.2x slower to 8 percent faster than base at 8 workers, shared-instance and per-instance modes are now identical, and removing the per-match allocations lets the function scale near linearly to 8 workers (7.1 to 72 Mrows/s at 8 workers, where base and the previous head were both stuck near 7). Outputs stay byte identical across base and both head builds in every cell. Fix verification tables and CSVs are in the gist as well. -- 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]
