Bind thread for cudastorage drop#82
Conversation
WalkthroughA Changes
Sequence Diagram(s)sequenceDiagram
participant Thread
participant CudaStorageSlice
participant CudaSlice
participant Device
Thread->>CudaStorageSlice: Drop (destructor called)
CudaStorageSlice->>CudaSlice: Access contained slice
CudaSlice->>Device: Retrieve device
Device->>Thread: Bind CUDA context to thread
Note right of Thread: Context is now set
CudaStorageSlice->>CudaSlice: Drop CudaSlice
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
candle-core/src/cuda_backend/mod.rs (1)
80-90: Clarify the commented alternative implementation.The helper function implementation looks correct, but the commented alternative approach (line 87) raises questions about which method is preferred for setting the CUDA context.
Consider either:
- Removing the commented line if
device.bind_to_thread()is the definitive approach- Adding a comment explaining why this alternative exists and when it might be preferred
- // OR: cudarc::driver::result::ctx::set_current(device.cu_primary_ctx())?; + // Alternative approach: cudarc::driver::result::ctx::set_current(device.cu_primary_ctx())?; + // Note: bind_to_thread() is preferred because [reason]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
candle-core/src/cuda_backend/mod.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Check (windows-latest, stable)
- GitHub Check: Test candle-book
🔇 Additional comments (1)
candle-core/src/cuda_backend/mod.rs (1)
61-78: Well-implemented Drop trait for CUDA context management.The Drop implementation correctly ensures that the proper CUDA context is set before each CudaSlice variant is dropped. Ignoring the error result in the drop method is the appropriate approach since drop implementations cannot propagate errors.
1 participant
Summary by CodeRabbit