andygrove commented on PR #6336: URL: https://github.com/apache/datafusion-comet/pull/6336#issuecomment-5876121871
This is a light fully automated review since there are so many PRs open. `buffer_bytes_held` at `native/shuffle/src/writers/local/local_partition_writer.rs:195` charges `codec_context.retained_bytes()`, and that workspace is allocated by libzstd through libc `malloc` (zstd-sys only swaps the allocator on wasm). So with this PR the pool tracks C library memory that the `AccountingAllocator` behind `native_allocated` never sees, and two docs now say the opposite. The "Non-Rust allocations" bullet at `docs/source/contributor-guide/memory_management.md:406` says neither the memory pool nor the allocation counters see such memory. Step 3 of "Sizing the Overhead from the Memory Usage Log" at `docs/source/user-guide/latest/tuning/memory.md:175` says neither `allocated` nor `reserved` includes memory from native C libraries such as zstd. When `spark.comet.shuffle.compression.codec` is `zstd`, `reserved` now holds one context per multi-partition shuffle task that is mid-spill or in its final write (about 1.3 MiB at the default level and close to 8 MiB at levels 7 and 8), and that also comes straight off the `allocated - reserved` difference the tuning page sizes the overhead from. Charging it still seems right to me, since it is real memory Spark should budget for. Could those two passages call out the shuffle writer's zstd context as the exception, alongside the list this PR already updates higher up in the same tuning page? -- 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]
