sunchao commented on code in PR #5991:
URL: https://github.com/apache/datafusion-comet/pull/5991#discussion_r4038835973
##########
native/common/src/bin/analyze_trace.rs:
##########
@@ -158,30 +231,14 @@ fn main() {
continue;
}
- // After each allocated or pool update, check the current state. A
comparison needs one
- // sample of each side: an observed zero reservation is a real value
that allocation can
- // exceed, so only the absence of any pool sample defers the check.
- let pool_total: u64 = pool_by_thread.values().sum();
- if pool_total > peak_pool_total {
- peak_pool_total = pool_total;
- }
-
- if source.is_some() && !pool_by_thread.is_empty() && latest_allocated
> pool_total {
- let excess = latest_allocated - pool_total;
- if excess > peak_excess {
- peak_excess = excess;
- }
- // Record violation (sample - don't record every single one)
- if violations.is_empty()
- || event.ts.saturating_sub(violations.last().unwrap().ts) >
1_000_000
- || excess == peak_excess
- {
- violations.push(MemorySnapshot {
- ts: event.ts,
- allocated: latest_allocated,
- pool_total,
- });
- }
+ // Legacy association, for traces recorded before the process-wide
total existed: compare
+ // the latest allocation against the running per-thread sum after
every counter event.
+ // There is nothing to pair on in those traces, so this keeps them
analyzable on the terms
+ // the tool always used, over-count and all.
+ let per_thread_sum: u64 = pool_by_thread.values().sum();
+ legacy.observe_total(per_thread_sum);
+ if latest_allocated > 0 && per_thread_sum > 0 {
+ legacy.compare(event.ts, latest_allocated, per_thread_sum);
Review Comment:
### Correctness
[P2] Preserve comparisons against observed zero reservations in legacy traces
`per_thread_sum > 0` treats a recorded zero as though no reservation sample
exists. For a legacy trace with `native_allocated=100 MiB`, then
`thread_1_comet_memory_reserved=100 MiB`, then that reservation dropping to
zero, both the base and previous head report 100 MiB of excess. This head
reports zero excess and `OK: native_allocated never exceeded the total pool
reservation.` A trace whose only reservation is zero instead incorrectly says
there are no pool samples. These are precisely the samples needed to detect
allocations retained after a pool releases memory. Gate the legacy comparison
on whether allocation and pool samples have been observed, rather than whether
their values are positive, and cover both zero cases.
--
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]
