peterxcli opened a new pull request, #5464:
URL: https://github.com/apache/datafusion-comet/pull/5464
## Which issue does this PR close?
Part of #5212 (finding #20 and the correctness portion of finding #19). This
does not close the parent audit issue.
## Rationale for this change
Tracing registers each execution context's memory pool in
`THREAD_MEMORY_POOLS`, but registration currently happens without a lifetime
guard. A failure during later plan creation or during the metrics update in
`releasePlan` skips unregistration and retains the pool indefinitely.
Debug tracing also registers each plan's distinct `LoggingMemoryPool`
wrapper. Two contexts sharing one 4,096-byte task pool therefore report 8,192
bytes instead of the 4,096-byte ground truth.
On unmodified upstream main, a deterministic failure reproducer left 100
registry entries after plan-creation failures and 200 after an additional 100
metrics failures. Those entries reported 819,200 bytes and retained 200 extra
references to the shared base pool.
## What changes are included in this PR?
- Register the task-shared base pool before constructing a per-plan logging
wrapper.
- Store registration in a drop guard so every post-registration creation
error removes its entry.
- Move the guard out before the fallible release metrics update, preserving
the raw execution context for the existing JVM retry while cleaning the tracing
registry on error.
- Add a focused regression covering two debug wrappers over one base pool,
exact reserved-byte deduplication, repeated creation/metrics failures, and
retained-reference cleanup.
The existing registry locking and scan algorithm are unchanged.
## How are these changes tested?
- `cargo test -p datafusion-comet --no-default-features
execution::jni_api::tests::thread_memory_pool_registration_is_scoped_and_deduplicates_base_pool
--profile ci -- --exact --nocapture --test-threads=1`
- 1 passed.
- `cargo test -p datafusion-comet --no-default-features --lib --profile ci
-- --test-threads=1 --skip parquet::objectstore::s3::tests::`
- 126 passed.
- `cargo clippy -p datafusion-comet --no-default-features --lib --tests
--profile ci -- -D warnings`
- `cargo fmt --check --all`
- `git diff --check`
An unfiltered library run passed 155 of 164 tests. Eight S3 credential tests
failed because this host has no usable native root certificates; a
representative failure reproduced with the unmodified-main binary. The
remaining order-sensitive cache-refresh failure passed independently on both
versions.
A warmed optimized A-B-B-A comparison used 31 interleaved samples per arm:
| Workload | Upstream main | This PR | Change |
| --- | ---: | ---: | ---: |
| Tiny, one context/pool | 95.4 ns/op | 98.9 ns/op | +3.6% |
| Two contexts sharing one pool | 110.5 ns/op | 109.0 ns/op | -1.3% |
| Registration lifecycle | 193.4 ns/op | 187.9 ns/op | -2.9% |
Per-arm IQRs overlapped for all three workloads.
--
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]
