You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Downgrade executor over-credits bytes_freed and wastes D2H on view-backed batches (pinned-scan views need an upstream cuCascade reclaimability predicate) #1594
PR #1582 serves unfiltered GPU-pinned scans as zero-copy views: the emitted data_batch holds a cudf::table_view into the pinned entry plus a type-erased keep-alive owner (emit_view_forward, src/op/scan/sirius_gpu_scan_operator.cpp:206-234), instead of the previous per-query deep copy.
Problem (verified against the branch)
The downgrade executor selects these view-backed batches as spill victims and credits their full accounted size as freed:
Both sweep tiers size victims via convertible_data_batch::bytes_in_space → get_size_in_bytes() (src/downgrade/downgrade_executor.cpp:236, :304), which for the view alternative returns the caller-stamped estimate of referenced pinned bytes.
Converting the victim pays cudf::pack (a transient D2D allocation that briefly increases GPU usage) + D2H + host allocation, then destroys only the keep-alive owner. The scan manager's pinned_entry still holds every shared_ptr<cudf::column>, so real GPU pool usage barely drops.
bytes_freed is nonetheless credited the full amount (:266, :329), falsely satisfying monitor-request predicates (:469) and request_free_memory predicates (:519). Pre-PR, the per-query copy was genuinely reclaimable, so this is a spill-effectiveness regression for pinned-scan outputs under memory pressure.
No correctness impact.convert_gpu_to_host synchronizes before the source representation dies; the in-place convert does release the batch's owner (the branch's own test asserts use_count() == 1 after convert); and the pipeline executor's allocation-shortfall path uses a ground-truth reservation-retry predicate, so it wastes sweeps but is not falsely satisfied. Only monitor requests and the request_free_memory API see phantom freed bytes.
Pre-existing scope. The same over-credit already applies to the older view-batch producers — projection passthrough/mixed outputs (src/op/sirius_physical_projection.cpp:137, :178) and table-scan projected views (src/op/sirius_physical_table_scan.cpp:248) — where only the freshly-evaluated component (if any) is genuinely reclaimable. PR #1582 makes the pattern systematic for pinned scans.
Why there is no small sirius-side fix
cuCascade is a pinned submodule, and gpu_table_representation keeps its unique_ptr<cudf::table> vs view + std::any owner variant private with no public predicate (cucascade/include/cucascade/cudf/gpu_data_representation.hpp:201-208). A sirius-only workaround requires a representation subclass plus forwarding converter registrations for every (source → target) pair (the registry keys on exact typeid), a new virtual on convertible_data, executor changes in both tiers, and a make_data_batch_from_view signature change across all six call sites — disproportionate for a warning-level accounting flaw, and it adds a latent "no converter registered" throw risk for any unanticipated conversion pair.
Recommended fix
Upstream cuCascade (~5 lines): add a [[nodiscard]] bool owns_table() const — or better, a virtual std::size_t reclaimable_bytes() const on idata_representation defaulting to get_size_in_bytes(), with the view alternative of gpu_table_representation returning a caller-stamped value (0 for pure forwards). This mirrors the existing view-aware branching in rebind_stream() / release_table().
Sirius (a few lines, after the submodule bump): expose reclaimable_bytes_in_space on convertible_data, exclude zero-reclaim batches from victim selection in both tiers (convertible_data_batch_provider::try_get_batch, convertible_gpu_pipeline_task_provider::has_matching_batches), and credit bytes_freed with reclaimable rather than accounted bytes. Mixed batches (scan cast columns, projection evaluated columns) keep their genuinely reclaimable component.
The team already tracks upstream cuCascade PRs through submodule bumps routinely (see CMakeLists.txt comments referencing cuCascade #126/#128/#130/#150).
Context
PR #1582 serves unfiltered GPU-pinned scans as zero-copy views: the emitted
data_batchholds acudf::table_viewinto the pinned entry plus a type-erased keep-alive owner (emit_view_forward,src/op/scan/sirius_gpu_scan_operator.cpp:206-234), instead of the previous per-query deep copy.Problem (verified against the branch)
The downgrade executor selects these view-backed batches as spill victims and credits their full accounted size as freed:
convertible_data_batch::bytes_in_space→get_size_in_bytes()(src/downgrade/downgrade_executor.cpp:236,:304), which for the view alternative returns the caller-stamped estimate of referenced pinned bytes.cudf::pack(a transient D2D allocation that briefly increases GPU usage) + D2H + host allocation, then destroys only the keep-alive owner. The scan manager'spinned_entrystill holds everyshared_ptr<cudf::column>, so real GPU pool usage barely drops.bytes_freedis nonetheless credited the full amount (:266,:329), falsely satisfying monitor-request predicates (:469) andrequest_free_memorypredicates (:519). Pre-PR, the per-query copy was genuinely reclaimable, so this is a spill-effectiveness regression for pinned-scan outputs under memory pressure.No correctness impact.
convert_gpu_to_hostsynchronizes before the source representation dies; the in-place convert does release the batch's owner (the branch's own test assertsuse_count() == 1after convert); and the pipeline executor's allocation-shortfall path uses a ground-truth reservation-retry predicate, so it wastes sweeps but is not falsely satisfied. Only monitor requests and therequest_free_memoryAPI see phantom freed bytes.Pre-existing scope. The same over-credit already applies to the older view-batch producers — projection passthrough/mixed outputs (
src/op/sirius_physical_projection.cpp:137,:178) and table-scan projected views (src/op/sirius_physical_table_scan.cpp:248) — where only the freshly-evaluated component (if any) is genuinely reclaimable. PR #1582 makes the pattern systematic for pinned scans.Why there is no small sirius-side fix
cuCascade is a pinned submodule, and
gpu_table_representationkeeps itsunique_ptr<cudf::table>vsview + std::any ownervariant private with no public predicate (cucascade/include/cucascade/cudf/gpu_data_representation.hpp:201-208). A sirius-only workaround requires a representation subclass plus forwarding converter registrations for every(source → target)pair (the registry keys on exacttypeid), a new virtual onconvertible_data, executor changes in both tiers, and amake_data_batch_from_viewsignature change across all six call sites — disproportionate for a warning-level accounting flaw, and it adds a latent "no converter registered" throw risk for any unanticipated conversion pair.Recommended fix
[[nodiscard]] bool owns_table() const— or better, a virtualstd::size_t reclaimable_bytes() constonidata_representationdefaulting toget_size_in_bytes(), with the view alternative ofgpu_table_representationreturning a caller-stamped value (0 for pure forwards). This mirrors the existing view-aware branching inrebind_stream()/release_table().reclaimable_bytes_in_spaceonconvertible_data, exclude zero-reclaim batches from victim selection in both tiers (convertible_data_batch_provider::try_get_batch,convertible_gpu_pipeline_task_provider::has_matching_batches), and creditbytes_freedwith reclaimable rather than accounted bytes. Mixed batches (scan cast columns, projection evaluated columns) keep their genuinely reclaimable component.The team already tracks upstream cuCascade PRs through submodule bumps routinely (see
CMakeLists.txtcomments referencing cuCascade #126/#128/#130/#150).References
fix/scan-gpu-pin-view-forward)src/op/scan/sirius_gpu_scan_operator.cpp:206-234(emit_view_forward)src/downgrade/downgrade_executor.cpp:236/266(tier 1),:304/329(tier 2),:469/:519(predicates)src/include/data/convertible_data_batch.hpp:168-173(bytes_in_space)src/scan_manager/sirius_scan_manager.cpp:205-247(pinned entry retains the columns)