Hash gpu_rtx mesh data without a full device-to-host copy; launch triangulation kernel once (#3691)#3693
Conversation
…iangulation once (#3691)
brendancol
left a comment
There was a problem hiding this comment.
PR Review: Hash gpu_rtx mesh data without a full device-to-host copy; launch triangulation kernel once (#3691)
Reviewed the full files, not just the diff, and ran the changed paths on a CUDA + rtxpy host (RTX A6000, OptiX 9.1.0).
Blockers (must fix before merge)
None found.
Suggestions (should fix, not blocking)
-
_data_hash(xrspatial/gpu_rtx/mesh_utils.py:48-49) hashes(data.shape, sample.get().tobytes())but not the dtype. Two rasters with the same shape and identical sampled bytes but different dtypes (say float32 vs int32 reinterpretations) would collide. Within xrspatial this cannot bite because both entry points build a freshRTX()whosegetHash()never matches, but an external caller reusing anRTXinstance across rasters would rely on this hash. Addingstr(data.dtype)to the tuple costs nothing.
Nits (optional improvements)
-
hash()on bytes is salted per process (PYTHONHASHSEED), sodatahashis not stable across runs. The oldhash(str(...))had the same property and the geometry cache lives in device memory, so nothing changes in practice; azlib.crc32orhashlib.blake2bdigest would make the value deterministic if the cache ever outlives the process. - In
_data_hashthe border slices and the strided interior sample overlap (row 0 and column 0 appear twice, corners three times). Harmless for hashing, just redundant bytes in the sample.
What looks good
- The single-launch rewrite is behavior-preserving: the kernel already guards
global_id < W*H, and 2^31-1 blocks covers any raster the_memory.pyguard admits. Verified hillshade and viewshed outputs on the same 1000x1000 scene are numerically identical to main. - Measured wins are real and reproduced locally: the hash drops from 27 ms to 0.14 ms on a 4000x4000 float32 raster, the triangulation phase from 38.2 ms to 1.5 ms, and end to end
hillshade_rtxgoes 288 -> 211 ms,viewshed_gpu278 -> 196 ms. TheNumbaPerformanceWarningabout grid size 100 is gone (confirmed with warnings-as-errors). - The new tests pin the two behaviors that could silently regress: corner sensitivity of the hash (which
benchmarks/benchmarks/common.pyrelies on to defeat mesh caching viaz[-1, -1]) and full-grid coverage past the old 102,400-thread batch boundary (400x300 test with NaN/-1 sentinel prefill). - The two tests that previously reproduced the old hash formula to mock a cache hit now call
_data_hashinstead of duplicating the expression. - The new
CreateTriangulationasv benchmark isolates the mesh build (hash + kernel + BVH) and skips cleanly viaNotImplementedErrorwhen rtxpy is absent, matching the existing rtxpy benchmark convention. - State CSV row is a clean one-line CRLF-preserving insertion.
Checklist
- Algorithm matches reference/paper (n/a; mesh geometry unchanged, outputs verified identical to main)
- All implemented backends produce consistent results (cupy/RTX path only; numpy triangulation path untouched; gpu_rtx has no dask backend)
- NaN handling is correct (all-NaN rasters still rejected by the pre-existing maxH guard before hashing)
- Edge cases are covered by tests (corner/interior/shape hash sensitivity, grid larger than the old batch, existing 4x3 and guard tests still pass)
- Dask chunk boundaries handled correctly (n/a, no dask path)
- No premature materialization or unnecessary copies (the full-raster D2H copy is what this PR removes; only a perimeter + strided sample crosses to the host now)
- Benchmark exists (new
benchmarks/benchmarks/gpu_rtx_mesh.py::CreateTriangulation) - README feature matrix updated (n/a, no new public function, no backend change)
- Docstrings present and accurate (
_data_hashdocstring explains the sampling and the benchmark corner contract)
Test runs on this host: test_gpu_rtx_mesh.py 14 passed, test_hillshade.py 56 passed, test_viewshed.py 120 passed / 1 skipped, test_gpu_rtx_memory.py + test_gpu_rtx_has_rtx.py + test_visibility.py 50 passed. flake8 clean on the changed files (the isort disagreement in test_gpu_rtx_mesh.py predates this PR).
…ic blake2b digest (#3691)
brendancol
left a comment
There was a problem hiding this comment.
PR Review (follow-up): commit 867e101
Re-reviewed after the review follow-up commit. Scope of the delta: _data_hash now feeds shape, dtype, and the sample bytes through hashlib.blake2b(digest_size=8) instead of the salted built-in hash(), plus two new tests.
Blockers (must fix before merge)
None found.
Suggestions (should fix, not blocking)
None.
Nits (optional improvements)
None. The overlapping-sample nit from the first review was dismissed with a reason I agree with: deduplicating the border/interior slices would add 1-row/1-column edge cases for no measurable gain.
Verification of the delta
- Both first-review findings are addressed: dtype now participates in the digest (Suggestion), and the value is deterministic across processes (Nit about PYTHONHASHSEED salting). One change resolves both.
digest_size=8yields exactly 8 bytes, soint.from_bytes(..., 'little')always fitsnp.uint64; the NumPy 2.0 out-of-bounds cast issue that #805 fixed cannot recur here.- The dtype test uses all-zero float32 vs int32 arrays, which are byte-identical, so it fails unless dtype is in the digest. The determinism test pins a golden value; I confirmed the same value is produced under two different PYTHONHASHSEED settings.
- Timing holds:
_data_hashon a 4000x4000 float32 raster is 0.24 ms with blake2b (0.14 ms with the salted hash, 27 ms before this PR). No effect on the end-to-end numbers in the PR body. - Full mesh suite passes (16 tests), hillshade + viewshed suites pass (176 passed, 1 skipped), flake8 clean on changed files.
Checklist
- Algorithm matches reference/paper (n/a; hash formula change only, mesh geometry untouched)
- All implemented backends produce consistent results (cupy/RTX path only)
- NaN handling is correct (unchanged; all-NaN rasters still rejected before hashing)
- Edge cases are covered by tests (dtype collision and cross-process determinism added)
- Dask chunk boundaries handled correctly (n/a)
- No premature materialization or unnecessary copies (sample-only D2H unchanged)
- Benchmark exists (
CreateTriangulation, unchanged by this commit) - README feature matrix updated (n/a)
- Docstrings present and accurate (docstring updated to describe the blake2b digest)
Nothing further; ready from my side.
Closes #3691
Two fixes on the
create_triangulation()path shared byhillshade(shadows=True)andviewshed()on cupy-backed rasters:hash(str(raster.data.get()))transferred the full array over PCIe (27 ms for a 4000x4000 float32 raster) and then hashed only numpy's truncated repr, i.e. corner and edge values. The new_data_hash()hashes the shape plus the exact bytes of the border rows/columns and a strided interior sample: 0.14 ms, and it now also detects interior-only changes the old hash was blind to. The border stays in the sample so the corner perturbationbenchmarks/benchmarks/common.pyuses to defeat mesh caching keeps working._triangulate_terrainnow launches the triangulation kernel once instead of batching it into 100-block launches. The old loop turned a 4000x4000 raster into 157 serialized, under-occupied launches (38.2 ms vs 1.48 ms for one launch) and triggered aNumbaPerformanceWarningabout grid size 100 on every call. CUDA's 1D grid limit is 2^31-1 blocks, so one launch covers any raster the_memory.pyguard admits, and the kernel already bounds-checksglobal_id < W*H.Measured end to end on an RTX A6000 (OptiX 9.1.0, 4000x4000 float32):
hillshade_rtx288 -> 211 ms,viewshed_gpu278 -> 196 ms. Output is unchanged: hillshade and viewshed results on the same scene are identical before and after (verified numerically against main), and the rtx-vs-CPU parity tests pass.Backends: only the cupy RTX/OptiX path is touched. numpy, dask+numpy, and dask+cupy paths are unaffected (gpu_rtx has no dask backend).
Also includes:
benchmarks/benchmarks/gpu_rtx_mesh.py::CreateTriangulationtiming the mesh build (hash, triangulation kernel, BVH) so a regression on this path shows up in benchmark runs.test_gpu_rtx_mesh.py:_data_hashdeterminism, corner/interior/shape sensitivity, and a 400x300 triangulation test that exceeds the old 102,400-thread batch so single-launch grid coverage is exercised.gpu_rtx(performance sweep bookkeeping).Test plan:
pytest xrspatial/tests/test_gpu_rtx_mesh.py(14 passed, includes the 5 new tests, run on CUDA + rtxpy hardware)pytest xrspatial/tests/test_hillshade.py(56 passed, includestest_hillshade_rtx_with_shadows)pytest xrspatial/tests/test_viewshed.py(120 passed, 1 skipped, includes the rtx GPU paths)pytest xrspatial/tests/test_gpu_rtx_memory.py xrspatial/tests/test_gpu_rtx_has_rtx.py xrspatial/tests/test_visibility.py(50 passed)