Fix segfault - #19
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe CUDA minimizer now resizes state and pointer buffers on both growth and shrinkage of problem topology. A regression test reuses one Gauss-Newton minimizer across problems with decreasing state-batch counts. ChangesMinimizer state batch reuse
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 14 UNAVAILABLE: read ECONNRESET Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cunls/minimizer/minimizer_state.cu (1)
70-90: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftAvoid freeing batch buffers on the shrink path.
states_.resize(state_batches.size())only makes the outer vector shorter, but that still destroys the droppedDeviceVectors, andDeviceVector::~DeviceVector()callscudaFree. The innerstate_vec.resize(size)is cheap when the new size fits capacity, so the stall risk here comes from recurring topology shrink itself; if that path is common, keep the removed buffers around or moveDeviceVectorto an async/pool-backed allocator.🤖 Prompt for 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. In `@cunls/minimizer/minimizer_state.cu` around lines 70 - 90, Update MinimizerState::CreateStates so shrinking the topology does not destroy dropped DeviceVectors or invoke cudaFree through their destructors. Preserve removed batch buffers for reuse, or replace their allocation with an async/pool-backed strategy, while retaining existing resizing behavior for active batches.Source: Path instructions
🤖 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.
Nitpick comments:
In `@cunls/minimizer/minimizer_state.cu`:
- Around line 70-90: Update MinimizerState::CreateStates so shrinking the
topology does not destroy dropped DeviceVectors or invoke cudaFree through their
destructors. Preserve removed batch buffers for reuse, or replace their
allocation with an async/pool-backed strategy, while retaining existing resizing
behavior for active batches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 62fab16a-e1f5-43f4-83bb-caa2ab233f22
📒 Files selected for processing (2)
cunls/minimizer/minimizer_state.cutests/gauss_newton_test.cpp
Summary by CodeRabbit
Bug Fixes
Tests