dwsmith1983 commented on PR #5615: URL: https://github.com/apache/datafusion-comet/pull/5615#issuecomment-5520439627
Took your preferred direction and it worked out well, thanks for the concrete numbers, they made the case obvious. Pushed in 36fa94d3a: - The scan key is now derived once on the driver and carried in the NativeScan proto (new `source_key` field, native ignores it). The executor reads it directly, so the keyCache LRU is deleted along with derivation after eviction. The shuffle writer path benefits too since the key rides inside `spec.childNativeOp`. - Prepared per-scan data now lives inside the base plan's own cache entry (`CachedPlanData` holding the parsed plan plus its prepared commons), so plans and their scans share one eviction unit and the standalone common cache is gone. The shuffle path gets an equivalent store scoped by shuffleId. - The base plan cache keys on a `PlanKey` that stores its hash, computed once per task outside the monitor. Driver transport for that one was not practical since the plan bytes are the task binary itself, so this is your middle option. - On your identity caveat: `sourceKey` does not cover the scalar subquery filters finalized after planning, so each prepared entry pins the exact finalized bytes it was parsed from and a hit is honored only on byte equality; a changed common under the same key re-prepares and replaces, never serves stale. There is a test proving the replacement. - The memo test now compares `getKey` with `sourceKey(common)` directly, per your correction. Benchmark with your requested matrix (component harness against the exact cache/injector code at three commits: main uncached, the previous design, this one; Apple M5, JDK 17, one forked JVM per cell, reuse counts observed by reference identity): Warm single-entry lookup, 42KB plan, us per lookup: | threads | uncached parse | previous | now | |---|---|---|---| | 1 | 111.6 | 29.9 | 28.2 | | 8 | 164.1 | 242.6 | 36.5 | | 32 | 500.0 | 2348.2 | 117.8 | Your churn shapes, warm pass, prepared commons reused: | shape | previous | now | |---|---|---| | 1 plan x 17 scans | 0/17 | 17/17 | | 9 plans x 2 scans | 0/18 | 18/18 | Warm pass time on the second shape drops 14.7ms to 1.0ms and allocation 61MB to 0.8MB per pass. Transported key read is 0.07us vs 0.4 to 0.55ms per derivation under churn. Shuffle steady state improves 1654 to 1354us per call with 1 prepare instead of 64 on the uncached base. Injected outputs are byte identical across all three commits for equal inputs and differ only in partition fields across different file sets. Retained heap per entry is unchanged between the two designs (about 241KB parsed plan, 567KB with a prepared wide common), the difference is eviction shape, not weight. One honest note: on a cold start with 8 threads racing the very first shuffle calls, concurrent callers can each prepare the same common once before the store converges (8 then 1 thereafter). Transient duplicate work only, the hot path stays lock free on hits. And yes, the new review format works well from this side: the measured tables made it unambiguous what to fix and in what order. -- 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]
