refactor(parquet): extract page_decode_setup_state substruct - #23471
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughParquet GPU decoding now stores page, column, range, sizing, and error metadata under ChangesParquet decode state migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/io/parquet/page_delta_decode.cu`:
- Around line 504-506: Update the use_char_ll heuristic in the page delta decode
setup to guard against s->setup.page.num_valids being zero. Select the
non-character-parallel path when there are no valid values, and only perform the
average-length division when num_valids is nonzero.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 62550d8d-e039-4670-b07d-ad2ae1629143
📒 Files selected for processing (8)
cpp/src/io/parquet/decode_fixed.cucpp/src/io/parquet/decode_preprocess.cucpp/src/io/parquet/page_data.cucpp/src/io/parquet/page_data.cuhcpp/src/io/parquet/page_decode.cuhcpp/src/io/parquet/page_delta_decode.cucpp/src/io/parquet/page_string_decode.cucpp/src/io/parquet/page_string_utils.cuh
|
@vyasr I think there's a comment from @coderabbitai that might be worth looking at. Also please note we might want to let #23314 merge (touches some common code in this PR) and apply changes to the new code as well |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="rapidsai/cudf"
echo "== PR `#23471` CodeRabbit review comments =="
gh api --paginate "repos/${repo}/pulls/23471/comments" \
--jq '.[] | select(.user.login == "coderabbitai[bot]" or .user.login == "coderabbitai") | {url: .html_url, path: .path, line: .line, body: .body}' || true
echo
echo "== PR `#23471` general CodeRabbit comments =="
gh api --paginate "repos/${repo}/issues/23471/comments" \
--jq '.[] | select(.user.login == "coderabbitai[bot]" or .user.login == "coderabbitai") | {url: .html_url, body: .body}' || true
echo
echo "== PR `#23314` status and changed files =="
gh pr view 23314 --repo "$repo" --json state,mergedAt,mergeable,baseRefName,headRefName,url,title \
--jq '{state, mergedAt, mergeable, baseRefName, headRefName, url, title}'
gh pr diff 23314 --repo "$repo" --name-onlyLength of output: 10772
PR ✏️ Learnings added
|
eab52ec to
3a6b48e
Compare
|
/merge |
…evels_kernel (#23479) This PR builds on #23471 to create a minimal struct for the `preprocess_levels` kernel. The primary addition in this PR is `page_decode_stream_state` struct encapsulating the level stream decoding components of `page_state_s`, which now has one of those structs as a member. `preprocess_levels` now uses a minimal `level_scan_state` struct, which only contains the two sub-structs required by that kernel. The net result is a savings of 560 bytes of shared memory in the kernel. Interestingly, on a couple of architectures (sm75 and sm86) we also saw a drop register usage by 8 (80->72 on sm75, 54->46 on sm86). That is hopefully indicative of the ancillary benefits of reducing shared memory usage as we will be able to incrementally reduce the complexity of the kernels and make it easier for the compiler to produce better optimized code (in this case presumably with better register reuse by eliding intermediate shared memory accesses that made live range counting harder). Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Bradley Dice (https://github.com/bdice) - Muhammad Haseeb (https://github.com/mhaseeb123) URL: #23479
…_page_decode_state replacing page_state_s (#23610) This PR completes the incremental narrowing pattern from #23471, #23479, #23495, and #23496 by retiring `page_state_s` entirely. It extracts `page_decode_nesting_state` (the nesting decode cache and pointer) and composes `full_page_decode_state` from `page_decode_setup_state`, `page_decode_stream_state`, `page_decode_nesting_state`, `page_decode_progress_state`, and `page_decode_output_state`. Every full-decode kernel (`decode_page_data`, `decode_split_page_data_kernel`, `decode_page_data_generic`, `decode_delta_binary`, `decode_delta_byte_array`, `decode_delta_length_byte_array`, `compute_string_page_bounds`, `compute_page_sizes`) now takes this composed state instead of the ad-hoc `page_state_s`, and the old struct is deleted. The composition also folds `page_state_s`'s stand-alone level-decoding fields (`lvl_start[2]`, `lvl_end`) into the already-existing `page_decode_stream_state::abs_lvl_start` / `abs_lvl_end` arrays, and drops the unused `first_output_value` field. Together with the composition's tighter layout this removes 32 bytes of shared memory from every full-decode kernel on every generated architecture, with no register-count regressions. A fresh `cuobjdump -res-usage` comparison against `upstream/main` shows the same 32-byte shmem reduction on all eight migrated kernels on every generated architecture. Register usage does not increase anywhere; one variant of `decode_split_page_data_kernel` on `sm_86` drops by 8 registers: | Kernel | Arch summary | shmem delta | register changes | |---|---|---:|---| | `decode_page_data` | all generated arch variants | -32 bytes | unchanged | | `decode_split_page_data_kernel` | all generated arch variants | -32 bytes | unchanged except `sm_86`: one template variant 48 -> 40 | | `decode_page_data_generic` | all generated arch variants (44 template variants) | -32 bytes | unchanged | | `compute_page_sizes` | all generated arch variants | -32 bytes | unchanged | | `compute_string_page_bounds` | all generated arch variants | -32 bytes | unchanged | | `decode_delta_binary` | all generated arch variants | -32 bytes | unchanged | | `decode_delta_byte_array` | all generated arch variants | -32 bytes | unchanged | | `decode_delta_length_byte_array` | all generated arch variants | -32 bytes | unchanged | Representative sm_80 measurements: | Kernel | shmem before | shmem after | delta | regs before | regs after | |---|---:|---:|---:|---:|---:| | `decode_page_data` | 4112 | 4080 | -32 | 56 | 56 | | `decode_split_page_data_kernel` | 4112 | 4080 | -32 | 56 | 56 | | `decode_page_data_generic` (min variant) | 2104 | 2072 | -32 | 62 | 62 | | `decode_page_data_generic` (max variant) | 13024 | 12992 | -32 | 64 | 64 | | `compute_page_sizes` | 3240 | 3208 | -32 | 32 | 32 | | `compute_string_page_bounds` | 3256 | 3224 | -32 | 32 | 32 | | `decode_delta_binary` | 2700 | 2668 | -32 | 56 | 56 | | `decode_delta_byte_array` | 5068 | 5036 | -32 | 72 | 72 | | `decode_delta_length_byte_array` | 3392 | 3360 | -32 | 64 | 64 | With this PR the `page_state_s` type is fully removed. All Parquet decode and preprocess kernels now use one of four purpose-built shared-memory states (`level_scan_state`, `string_size_scan_state`, `string_offset_scan_state`, `full_page_decode_state`), each holding only the substructs it actually needs. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Bradley Dice (https://github.com/bdice) URL: #23610
Description
This PR looks large, but is entirely trivial. It simply moves some fields of
page_state_sinto a new struct, and an instance of that new struct is added topage_state_s, requiring every call site to now access the field through that struct member. Once this PR is merged, subsequent PRs will be able to introduce other such structs while simultaneously modifying device functions and kernels to actually reduce the shared memory footprint of those kernels (and eventually, to simplify the implementation of some of these common helper device functions), but this first PR is purely setup for that.Checklist