sunchao commented on PR #5565:
URL:
https://github.com/apache/datafusion-comet/pull/5565#issuecomment-5534294467
Reviewed **`1372d786`**** against ****`ef62b463`**. The overall
design is sound, and I found no new blocking defect. I would keep context
reuse, with these refinements:
1. **Release decoder memory at EOF.**\
The EOF branch leaves the decoder context populated. Plan and stream
clones share it, so a finished input can retain its workspace while other parts
of the task continue. Clearing the context at definitive EOF preserves all
useful reuse and shortens memory retention. The 8 MiB cap bounds this cost but
does not eliminate it. [EOF handling (line
185)](/home/dev-user/code/reviews/comet-pr5565-design-20260904/native/core/src/execution/operators/shuffle_scan.rs:185)
2. **Combine the batch and decoder under one mutex.**\
The new decoder mutex is only acquired while the existing batch mutex is
already held. It provides no additional concurrency in the current call graph.
A single `Arc<Mutex<ScanInputState>>`, containing the batch and decoder, would
preserve sharing across clones while removing an allocation per operator, an
extra lock per fetched block, and a second synchronization invariant. This is a
concrete simplification; its performance magnitude remains unmeasured. [State
and locking (line
141)](/home/dev-user/code/reviews/comet-pr5565-design-20260904/native/core/src/execution/operators/shuffle_scan.rs:141)
3. **Improve the benchmark’s coverage of the actual design.**\
Its “small” frames contain 8,192 rows, and it calls the decoder directly,
bypassing the operator mutex. The reusable context also survives across
benchmark iterations. That measures steady-state decoding, but cannot establish
reader startup/cleanup costs or operator overhead. Add the previously measured
400-row case, zstd levels 1/3, an LZ4 control, and a finite reader lifetime. An
operator-path benchmark would address the remaining overhead question.
[Benchmark (line
143)](/home/dev-user/code/reviews/comet-pr5565-design-20260904/native/shuffle/benches/ipc_decode.rs:143)
4. **Correct the supposedly fresh decoder in the reuse test.**\
Its `fresh` result comes from `read_ipc_compressed`, which now uses the
thread-local cached context. The test therefore compares two reused contexts.
Create a new `ShuffleDecodeContext` for each reference decode so the test
exercises its stated comparison. [Test (line
331)](/home/dev-user/code/reviews/comet-pr5565-design-20260904/native/shuffle/src/ipc.rs:331)
The remaining abstractions mostly earn their place:
- **Task-owned encoder state with mutable borrowing is appropriate.**
Putting contexts inside per-partition writers would multiply retained memory
and lose reuse across small partitions.
- **The spill-completion hook is reasonable.** It expresses a lifecycle
boundary without teaching the partitioner about zstd, and current callers
invoke it on both success and error.
- **Resetting the session and applying the level are justified.** The zstd
constructor does neither. Caching the last level would add bookkeeping for an
unmeasured, small saving.
- **RSS’s per-invocation release should remain.** Longer reuse requires
changing its memory-reservation contract. The existing context types and thin
decoder wrappers are preferable to introducing a general codec pool or
framework.
One optional performance follow-up: zstd’s Rust wrapper still allocates a
**32 KiB output buffer per frame**. This predates the PR. Profile that
allocation before adding lower-level streaming machinery to reuse it.
[Allocation (line
49)](/home/dev-user/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/zstd-0.13.3/src/stream/zio/writer.rs:49)
This was a fresh source/design review. The measured codec paths and lockfile
are unchanged from our earlier benchmarked revision; I did not rerun benchmarks
or tests. Current CI has **64 successful checks**, with the performance check
skipped.
--
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]
