andygrove commented on PR #5991: URL: https://github.com/apache/datafusion-comet/pull/5991#issuecomment-5716670476
Thanks — all three are real, fixed in c23468e9. On pool coverage you're right that the task-shared registry is the wrong source. I've moved the total onto `THREAD_MEMORY_POOLS`, deduplicated by pool identity across the whole map rather than within one thread. Every plan registers whichever pool `create_memory_pool` gave it, so the independent and global modes are covered, and a shared pool is still counted once. It also makes the total consistent with the per-thread counters by construction, since it's the deduplicated union of exactly the pools those counters come from. That removes the deadlock rather than fixing it — `total_reserved_across_tasks` is gone, so nothing upgrades a `Weak` under the registry lock any more and nothing can be dropped inside the critical section. So I haven't added the last-owner race test; there's no upgrade path left to race. Happy to add one if you'd rather keep a guard there. On sample pairing, confirmed. I built your sequence as a fixture and the old code reports a false 100 MB peak where the base reports zero. The analyzer now holds the allocation sample per tid and only compares when that thread's total arrives. Legacy traces keep the old per-event comparison — I diffed the two tools over a 4k-event trace and every number matches. -- 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]
