implement graph #16
implement graph #16
Conversation
WalkthroughThis pull request introduces significant enhancements to the graph module, focusing on the implementation of a Foreign Function Interface (FFI) for external language interaction. Key changes include the addition of various functions for managing graph entities, improved concurrency control, and refined data structures for matrices and tensors. The introduction of new types and methods facilitates better memory management and operations on nodes, edges, and matrix data. Additionally, a new Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 42
🧹 Outside diff range and nitpick comments (23)
src/graph/mod.rs (1)
6-8: LGTM: Module structure aligns with PR objectiveThe changes to this file align well with the PR objective of implementing a graph. The addition of the FFI module and making the graph module public are logical steps in this implementation.
A few points to consider:
- Ensure that the
ffimodule is properly implemented and follows best practices for FFI in Rust.- Verify that the public interface of the
graphmodule provides all necessary functionality for graph operations.- Consider adding documentation comments to explain the purpose of each module, especially for the new
ffimodule.As the project grows, consider organizing the FFI-related code into a separate top-level module if it's used by multiple components. This can help maintain a clean separation between core logic and FFI bindings.
src/binding/mod.rs (2)
6-7: LGTM! Consider improving module organization and documentation.The new module declarations for
cmutexandcrwlockare syntactically correct and follow Rust's naming conventions. These additions suggest an expansion of concurrency-related functionality in the binding.Consider the following improvements:
- Reorder the module declarations alphabetically for better organization:
pub mod cmutex; pub mod crwlock; pub mod graph;
- Add brief comments explaining the purpose of these new modules. For example:
// Custom mutex implementation pub mod cmutex; // Custom read-write lock implementation pub mod crwlock; // Graph-related functionality pub mod graph;These changes would enhance the readability and maintainability of the code.
Line range hint
1-9: Consider adding module-level documentation.The file structure is clean and follows common practices for Rust module files. To further improve the code's documentation, consider adding a module-level documentation comment at the beginning of the file, explaining the purpose of the
bindingmodule and its contents.Here's an example of how you could add module-level documentation:
/* * Copyright FalkorDB Ltd. 2023 - present * Licensed under the Server Side Public License v1 (SSPLv1). */ //! The `binding` module provides various bindings and utilities for the FalkorDB project. //! //! This module includes implementations for custom concurrency primitives and graph-related //! functionality. pub mod cmutex; pub mod crwlock; pub mod graph;This addition would provide users of the module with a quick overview of its contents and purpose.
src/graph/matrix/mod.rs (1)
6-12: Summary: Expanded public API for graph implementationThe changes in this file consistently make previously private modules (
GraphBLAS,delta_matrix_iter,sparse_matrix_iter) public and add a new publictensormodule. This expansion of the public API aligns with the PR objective of implementing graph functionality.While these changes are approved, they represent a significant modification to the module's interface. This could have far-reaching implications for the project's architecture and how it's used by other parts of the codebase or external consumers.
Consider the following recommendations:
- Document the newly public modules thoroughly, explaining their purpose and usage.
- Update any existing documentation or README files to reflect these API changes.
- Consider creating or updating integration tests to ensure the newly public modules work correctly when used from other parts of the codebase.
- If this is a breaking change, ensure it's properly communicated in release notes or changelog.
- Review the internal structure of each newly public module to ensure it's ready for public consumption (e.g., proper error handling, clear naming conventions, etc.).
Would you like assistance in implementing any of these recommendations?
src/binding/cmutex.rs (3)
10-14: Enhance documentation with safety implications.The struct definition and its purpose are well-documented. However, it would be beneficial to add more details about the safety implications of using this C mutex wrapper in Rust.
Consider expanding the documentation to include:
- Why Rust's native
Mutexcan't be used in this scenario.- The potential risks associated with using unsafe code and C bindings.
- Any precautions users should take when using this struct.
Example:
/// Wrap C mutex as we can't use Rust Mutex in this context. /// Used to lock the matrix only when we apply pending changes. /// /// # Safety /// This struct uses unsafe code to interact with C-style mutexes. /// Users should ensure proper usage to avoid undefined behavior. /// Prefer Rust's native `Mutex` when possible for better safety guarantees. pub struct CMutex { mutex: pthread_mutex_t, }
40-44: Consider logging errors in theDropimplementation.The
Dropimplementation correctly callspthread_mutex_destroyto clean up the mutex. However, it might be beneficial to log any errors that occur during destruction, even though we can't really handle them inDrop.Consider updating the implementation to log any errors:
impl Drop for CMutex { fn drop(&mut self) { unsafe { let result = libc::pthread_mutex_destroy(&mut self.mutex); if result != 0 { eprintln!("Failed to destroy mutex: {}", std::io::Error::from_raw_os_error(result)); } } } }This will print an error message to stderr if the mutex destruction fails, which could be helpful for debugging.
1-44: Overall assessment: Good start, needs safety improvementsThe implementation of
CMutexas a wrapper for C-style mutexes is a good approach for scenarios where Rust's nativeMutexcan't be used. However, there are several areas where the implementation can be improved:
- Proper use of
unsafe: Methods using unsafe C functions should be marked asunsafe.- Error handling: Add error checking and propagation for C function calls.
- Documentation: Enhance comments to explain safety implications and usage guidelines.
- Method signatures: Consider using
&selfinstead of&mut selfforlockandunlock.- Logging: Add error logging in the
Dropimplementation.These improvements will enhance the safety, reliability, and usability of the
CMutexstruct. Once these changes are implemented, the code will be much more robust and align better with Rust's safety principles while still providing the necessary low-level control.build.rs (1)
Line range hint
21-29: Conditional logic for CLANG looks good, but consider improving portability.The addition of conditional logic to handle CLANG-specific linker arguments is a good improvement. However, there are a few points to consider:
The hardcoded paths for LLVM libraries might not be portable across different systems. Consider using environment variables or build-time detection for these paths.
The script is linking against both LLVM 17 and 18 libraries. This might lead to compatibility issues or unnecessary dependencies. Consider specifying a single LLVM version or making it configurable.
Consider refactoring the LLVM library path handling for better portability:
if clang == "1" { let llvm_lib_path = env::var("LLVM_LIB_PATH").unwrap_or_else(|_| { // Default paths, preferably detected at build time "/usr/lib/llvm-18/lib".to_string() }); println!("cargo:rustc-link-arg=-L{}", llvm_lib_path); println!("cargo:rustc-link-arg=-lomp"); } else { println!("cargo:rustc-link-arg=-lgomp"); }This approach allows for more flexibility and easier configuration across different systems.
src/binding/crwlock.rs (3)
6-14: LGTM! Consider adding more detailed documentation.The imports and struct definition look good. The use of
Box<pthread_rwlock_t>ensures proper heap allocation for FFI.Consider adding more detailed documentation for the
CRWLockstruct, including:
- Why it's needed (inability to use Rust's RWLock)
- Its thread-safety properties
- Any limitations or considerations when using it
49-53: Consider adding error checking in Drop implementation.While
pthread_rwlock_destroyshouldn't fail for a valid rwlock, adding adebug_assert!would be consistent with the error checking in other methods. This could help catch potential issues during development.Consider modifying the
dropimplementation:impl Drop for CRWLock { fn drop(&mut self) { let result = unsafe { libc::pthread_rwlock_destroy(self.rwlock.as_mut()) }; debug_assert!(result == 0, "pthread_rwlock_destroy failed"); } }This change maintains consistency with the error checking approach used in other methods.
1-53: Overall feedback: Good start, but needs improvements in error handling and API design.The implementation provides a functional Rust wrapper for
pthread_rwlock_t, which is useful for scenarios where Rust's built-inRwLockcan't be used. However, there are several areas for improvement:
Error Handling: Implement proper error handling and propagation for all C FFI calls.
API Design: Reconsider the use of
&mut selfin methods where it might be unnecessarily restrictive.Documentation: Add comprehensive documentation, including:
- Explanation of why this wrapper is needed
- Thread-safety guarantees
- Potential deadlock scenarios and how to avoid them
- Best practices for using this rwlock
Consider implementing
SendandSynctraits explicitly to clearly define the thread-safety properties ofCRWLock.You might want to add methods for try_read_lock and try_write_lock (wrapping pthread_rwlock_tryrdlock and pthread_rwlock_trywrlock) to provide non-blocking alternatives.
These improvements will make the
CRWLockmore robust, easier to use correctly, and better integrated with Rust's error handling and concurrency models.src/graph/matrix/delta_matrix_iter.rs (1)
32-45: LGTM! Consider a minor style improvement.The new
new_rangemethod is well-implemented and provides a useful way to create aDeltaMatrixIterwith a specific row range. The implementation is correct and consistent.Consider simplifying the struct initialization by using the field shorthand notation:
- min_row: min_row, - max_row: max_row, + min_row, + max_row,src/graph/matrix/ffi.rs (2)
Line range hint
220-245: Use appropriate error codes for accurate error handlingIn
Delta_MatrixTupleIter_next_BOOLandDelta_MatrixTupleIter_next_UINT64, returningGrB_Info::GrB_NULL_POINTERfor all errors other thanOk(None)may not accurately represent the error conditions. Consider mapping specific errors to correspondingGrB_Infocodes for better error reporting.For example, you might adjust the error handling as follows:
match (*iter).next_bool() { Ok(Some((r, c))) => { // ... GrB_Info::GrB_SUCCESS } Ok(None) => GrB_Info::GxB_EXHAUSTED, Err(e) => { match e { SpecificErrorType => GrB_Info::GrB_INVALID_OBJECT, _ => GrB_Info::GrB_UNKNOWN, } } }
Line range hint
15-273: Consolidate#[allow(non_snake_case)]attributes to reduce repetitionThe
#[allow(non_snake_case)]attribute is applied before each function to suppress naming convention warnings. To reduce code duplication and improve maintainability, consider applying this attribute at the module level or grouping these functions within a scope where the attribute applies to all of them.You can place the attribute at the beginning of the file or module:
#![allow(non_snake_case)]src/graph/ffi.rs (3)
37-39: Consider adding safety documentation forGraph_AcquireReadLock.The function
Graph_AcquireReadLockuses unsafe code and operates on raw pointers. It's important to document the safety requirements and assumptions for this function to ensure correct usage by callers.Consider adding comments explaining the safety requirements for
g:/// Acquires a read lock on the graph. /// /// # Safety /// /// - `g` must be a valid, non-null pointer to a `Graph` instance. /// - The graph must be properly initialized before calling this function. unsafe extern "C" fn Graph_AcquireReadLock(g: *mut Graph) { (&mut *g).acquire_read_lock(); }
43-45: Consider adding safety documentation forGraph_AcquireWriteLock.Similar to
Graph_AcquireReadLock, it's important to document the safety requirements forGraph_AcquireWriteLock.Add safety comments for
g:/// Acquires a write lock on the graph. /// /// # Safety /// /// - `g` must be a valid, non-null pointer to a `Graph` instance. /// - The graph must be properly initialized before calling this function. unsafe extern "C" fn Graph_AcquireWriteLock(g: *mut Graph) { (&mut *g).acquire_write_lock(); }
50-51: Consider adding safety documentation forGraph_ReleaseLock.Ensure that the safety considerations for
Graph_ReleaseLockare documented.Include safety comments:
/// Releases the lock on the graph. /// /// # Safety /// /// - `g` must be a valid, non-null pointer to a `Graph` instance that was previously locked. unsafe extern "C" fn Graph_ReleaseLock(g: *mut Graph) { (&mut *g).release_lock(); }src/graph/matrix/tensor.rs (5)
24-26: Add documentation for thesingle_edgefunction.The function
single_edgechecks if the most significant bit (MSB) ofcurrent_edgeis not set. Adding a doc comment explaining its purpose and how it relates to edge representation would improve code readability and maintainability.
28-30: Add documentation for theset_msbfunction.The
set_msbfunction sets the most significant bit of anEdgeID. Providing a doc comment would help clarify its role in marking edge representations.
32-34: Add documentation for theclear_msbfunction.The
clear_msbfunction clears the most significant bit of anEdgeID. Including a doc comment explaining its purpose would enhance code clarity.
36-38: Add documentation comments for theTensorstruct.Adding Rust doc comments to the
Tensorstruct will improve code readability and help other developers understand its purpose and usage.
40-283: Add documentation comments for public methods inTensorimplementation.Adding Rust doc comments (
///) to the public methods of theTensorstruct, such asset_element,set_elements,remove_elements,iter, and others, will improve code readability and help users understand how to use these methods.src/graph/matrix/GraphBLAS.rs (1)
Line range hint
1-3: Ensure safe exposure of__BindgenComplextypesThe types
GxB_FC32_tandGxB_FC64_tare being publicly defined as aliases for__BindgenComplex<f32>and__BindgenComplex<f64>, respectively. Since__BindgenComplexis generated bybindgenand primarily intended for internal FFI use, exposing it publicly may lead to unintended usage or safety issues.Consider wrapping these complex types in safer, more idiomatic Rust abstractions or providing a safe interface for users to interact with complex numbers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
- build.rs (1 hunks)
- src/binding/cmutex.rs (1 hunks)
- src/binding/crwlock.rs (1 hunks)
- src/binding/graph.rs (4 hunks)
- src/binding/mod.rs (1 hunks)
- src/graph/ffi.rs (1 hunks)
- src/graph/graph.rs (1 hunks)
- src/graph/matrix/GraphBLAS.rs (1 hunks)
- src/graph/matrix/delta_matrix.rs (5 hunks)
- src/graph/matrix/delta_matrix_iter.rs (1 hunks)
- src/graph/matrix/ffi.rs (8 hunks)
- src/graph/matrix/mod.rs (1 hunks)
- src/graph/matrix/tensor.rs (1 hunks)
- src/graph/mod.rs (1 hunks)
- src/lib.rs (1 hunks)
- src/undo_log/undo_log.rs (2 hunks)
🔇 Additional comments (26)
src/graph/mod.rs (1)
7-7: Graph module made publicThe
graphmodule is now declared as public (pub mod graph;). This change aligns with the PR objective of implementing a graph, as it makes the graph functionality accessible to other parts of the codebase.Let's verify the contents and usage of the public graph module:
#!/bin/bash # Description: Check the contents and usage of the public graph module # Test 1: Check if the graph.rs file exists echo "Checking for graph.rs file:" fd --type f "graph.rs" src/graph # Test 2: Look for public functions or structs in the graph module echo "Searching for public declarations in the graph module:" rg --type rust "pub (fn|struct|enum)" src/graph/graph.rs # Test 3: Check for usage of the graph module in other files echo "Checking for usage of the graph module:" rg --type rust "use (crate::)?graph::graph::" srcsrc/graph/matrix/mod.rs (3)
11-12: Approved: sparse_matrix_iter made public and new tensor module addedThe change to make sparse_matrix_iter public aligns with the previous changes and likely supports the graph implementation. The addition of a new public tensor module suggests new functionality related to tensors.
To ensure these changes don't unintentionally expose internal implementation details or break existing code, please run the following verification script:
Could you please provide more information about the new tensor module? What functionality does it provide, and how does it relate to the graph implementation?
✅ Verification successful
Verified: Changes to
sparse_matrix_iterand addition oftensormodule are appropriateThe
sparse_matrix_itermodule has been correctly made public, and the newtensormodule is well-implemented to support tensor operations within the graph matrix. No unintended exposures or issues were found based on the verification results.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any existing usages of sparse_matrix_iter and verify the new tensor module # Test: Search for sparse_matrix_iter usages echo "Searching for sparse_matrix_iter usages:" rg --type rust "use .*sparse_matrix_iter" -g '!src/graph/matrix/mod.rs' # Test: Check if there are any visibility modifiers on items within sparse_matrix_iter echo "Checking for visibility modifiers within sparse_matrix_iter:" rg --type rust "pub (struct|enum|fn|trait|type)" src/graph/matrix/sparse_matrix_iter.rs # Test: Verify the contents of the new tensor module echo "Checking the contents of the new tensor module:" cat src/graph/matrix/tensor.rsLength of output: 15669
6-6: Approved: GraphBLAS module made publicThe change to make the GraphBLAS module public aligns with the PR objective of implementing a graph functionality. This will allow other parts of the codebase to access GraphBLAS features.
To ensure this change doesn't unintentionally expose internal implementation details or break existing code, please run the following verification script:
✅ Verification successful
Verified: GraphBLAS module is correctly made public
The
GraphBLASmodule has been successfully made public without introducing any issues. Existing usages insparse_matrix.rsare consistent with this change, and all necessary components withinGraphBLAS.rshave appropriate visibility modifiers.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any existing usages of GraphBLAS that might be affected by this change # Test: Search for GraphBLAS usages echo "Searching for GraphBLAS usages:" rg --type rust "use .*GraphBLAS" -g '!src/graph/matrix/mod.rs' # Test: Check if there are any visibility modifiers on items within GraphBLAS echo "Checking for visibility modifiers within GraphBLAS:" rg --type rust "pub (struct|enum|fn|trait|type)" src/graph/matrix/GraphBLAS.rsLength of output: 36552
8-8: Approved: delta_matrix_iter module made publicMaking the delta_matrix_iter module public is likely necessary for implementing graph functionality, possibly for iterating over delta matrices in other parts of the codebase.
To ensure this change doesn't unintentionally expose internal implementation details or break existing code, please run the following verification script:
✅ Verification successful
Action Required: Correct Verification Scripts
The previous verification scripts mistakenly referenced
sparse_matrix_iterinstead ofdelta_matrix_iter. Please use the corrected scripts below to ensure accurate verification of thedelta_matrix_itermodule:
Verified:
delta_matrix_iterModule Public AccessThe
delta_matrix_itermodule has been successfully made public without affecting other parts of the codebase. No external usages were found, and all intended items within the module are appropriately exposed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any existing usages of delta_matrix_iter that might be affected by this change # Test: Search for delta_matrix_iter usages echo "Searching for delta_matrix_iter usages:" rg --type rust "use .*delta_matrix_iter" -g '!src/graph/matrix/mod.rs' # Test: Check if there are any visibility modifiers on items within delta_matrix_iter echo "Checking for visibility modifiers within delta_matrix_iter:" rg --type rust "pub (struct|enum|fn|trait|type)" src/graph/matrix/delta_matrix_iter.rsLength of output: 15669
Script:
#!/bin/bash # Description: Check for any existing usages of delta_matrix_iter that might be affected by this change # Test: Search for delta_matrix_iter usages echo "Searching for delta_matrix_iter usages:" rg --type rust "use .*delta_matrix_iter" -g '!src/graph/matrix/mod.rs' # Test: Check if there are any visibility modifiers on items within delta_matrix_iter rg --type rust "pub (struct|enum|fn|trait|type)" src/graph/matrix/delta_matrix_iter.rsLength of output: 684
src/binding/cmutex.rs (1)
1-9: LGTM: Appropriate file header and imports.The file header contains the necessary copyright and license information. The imports from the standard library and libc are relevant for the implementation of the C-style mutex wrapper.
build.rs (3)
1-4: LGTM: Copyright notice added.The addition of the copyright notice is a good practice. It clearly states the ownership and licensing terms of the code.
11-11: LGTM: CLANG environment variable check added.The addition of the CLANG environment variable check is consistent with the existing checks and ensures the build script reruns when necessary.
Line range hint
1-32: Overall, the changes look good with some suggestions for improvement.The modifications to
build.rsenhance the build script's flexibility for different compiler environments, particularly with the addition of CLANG-specific handling. The copyright notice and environment variable checks are well-implemented.The main area for potential improvement is in the handling of LLVM library paths, which could be made more portable and configurable. Consider implementing the suggestions provided earlier to enhance the script's adaptability across different systems.
These changes align well with the PR objective of implementing a graph, as they seem to be setting up the necessary build environment for the graph implementation.
src/binding/crwlock.rs (2)
41-46:⚠️ Potential issueImprove error handling and consider method signature for release.
Similar to the other methods,
releaseshould handle errors properly and propagate them to the caller.The method takes
&mut self, which might be unnecessarily restrictive. Unlocking a rwlock doesn't require exclusive access to the lock itself.Suggested improvements:
- Error handling:
pub fn release(&self) -> Result<(), std::io::Error> { let result = unsafe { libc::pthread_rwlock_unlock(self.rwlock.as_ref() as *const _ as *mut _) }; if result != 0 { Err(std::io::Error::from_raw_os_error(result)) } else { Ok(()) } }
- Change the method signature to
&selfinstead of&mut self.To ensure that changing to
&selfdoesn't break any existing code, let's search for uses ofrelease:rg "\.release\(\)" --type rust
27-32:⚠️ Potential issueImprove error handling and consider method signature.
The current implementation uses
debug_assert!, which only checks in debug builds. Consider handling errors properly and propagating them to the caller.The method takes
&mut self, which is more restrictive than necessary for a read lock. This prevents multiple read locks from being acquired simultaneously, which is typically allowed for reader-writer locks.Suggested improvements:
- Error handling:
pub fn acquire_read(&self) -> Result<(), std::io::Error> { let result = unsafe { libc::pthread_rwlock_rdlock(self.rwlock.as_ref() as *const _ as *mut _) }; if result != 0 { Err(std::io::Error::from_raw_os_error(result)) } else { Ok(()) } }
- Change the method signature to
&selfinstead of&mut selfto allow multiple simultaneous read locks.To ensure that changing to
&selfdoesn't break any existing code, let's search for uses ofacquire_read:✅ Verification successful
Verification Successful: Safe to Refactor
acquire_readMethod SignatureThe
acquire_readmethod is only invoked with mutable references (&mut self) in the codebase. Changing the method signature to&selfwill not disrupt existing functionalities and will enable multiple simultaneous read locks as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg "acquire_read" --type rustLength of output: 82
Script:
rg "acquire_read" --type rustLength of output: 257
Script:
rg "acquire_read" --type rust -A 2Length of output: 486
src/lib.rs (1)
15-15: LGTM: Addition ofRedisModule_ReallocfunctionThe addition of the
RedisModule_Reallocfunction complements the existing memory management functions and allows for resizing of allocated memory. TheOption<...>wrapper is consistent with other Redis module functions, allowing for graceful handling if the function is not available.Suggestion: Implement
reallocmethod inFalkorDBAllocConsider implementing a
reallocmethod in theFalkorDBAllocstruct to utilize this new function. This would provide a complete set of memory management operations (alloc, realloc, dealloc) for the allocator.Here's a suggested implementation:
unsafe impl GlobalAlloc for FalkorDBAlloc { // ... existing alloc and dealloc methods ... unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { match RedisModule_Realloc { Some(realloc) => realloc(ptr.cast(), new_size).cast(), None => panic!("realloc function not found"), } } }Query: Relevance to PR objective
The PR objective mentions implementing a graph, but this change seems unrelated to that goal. Could you clarify how this memory management enhancement contributes to the graph implementation?
src/graph/matrix/ffi.rs (5)
Line range hint
17-24: FunctionDelta_Matrix_newimplementation is correctThe function correctly allocates a new
DeltaMatrixand initializes it with the provided parameters.
Line range hint
64-71: FunctionDelta_Matrix_setElement_BOOLimplementation looks goodThe function correctly sets a boolean element in the matrix at the specified indices and returns the appropriate success code.
93-100: FunctionDelta_mxmimplementation is appropriateThe matrix multiplication function correctly calls
mxmon theDeltaMatrixand handles the pointers safely.
105-112: FunctionDelta_eWiseAddimplementation is correctThe element-wise addition function properly invokes
element_wise_addand manages the inputs and outputs correctly.
124-129: FunctionDelta_Matrix_copyimplementation is accurateThe function correctly copies the contents of one
DeltaMatrixto another.src/binding/graph.rs (3)
8-8: Import statement is appropriateThe
usestatement correctly imports theGraphstruct fromcrate::graph::graph, enabling access to theGraphtype within this module.
62-68: Addition ofGraphEntitystructThe
GraphEntitystruct provides a general representation for graph entities, mirroring the structure of the existingNodestruct. This addition enhances code clarity and promotes reusability.
10-12: Verify the impact of changing ID types tou64Changing
NodeID,EdgeID, andEntityIDfromi64tou64ensures that identifiers are non-negative, which aligns with typical usage. However, ensure that all parts of the codebase that use these types are updated accordingly, and that there are no dependencies on negative values.Run the following script to identify potential issues:
✅ Verification successful
Impact verification successful: No issues found with changing ID types to
u64. All usages are updated accordingly, and no dependencies on negative values detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of NodeID, EdgeID, and EntityID that may assume signed integers. # Search for any casts or operations that could be affected by the change. rg --type rust --word-regexp "(NodeID|EdgeID|EntityID)" --context 3Length of output: 12310
src/undo_log/undo_log.rs (1)
268-272: Verify the logic of settingidtou64::MAXand associated assertionsIn the rollback process for deleted nodes and edges, the
idfields are set tou64::MAX:
- Line 268:
id: u64::MAX,- Line 280:
id: u64::MAX,Then, assertions are made:
- Line 270:
debug_assert!(*node_id >= node.id);- Line 288:
debug_assert!(*edge_id >= edge.id);Since
u64::MAXis the maximum possible value, these assertions will likely fail unless*node_idor*edge_idare alsou64::MAX. Please verify that this logic achieves the intended behavior and adjust if necessary.Also applies to: 280-288
src/graph/ffi.rs (1)
227-232: Possible incorrect deletion check inGraph_EntityIsDeleted.The function checks if
attributesis null and returnsfalseif it is. This may not correctly represent the deletion status of the entity.Please verify that the logic for checking if a
GraphEntityis deleted is correct. The current implementation may not accurately reflect the deletion status.src/graph/matrix/delta_matrix.rs (3)
8-11: Ensure the correct module path forCMuteximportThe code imports
CMutexfromcrate::binding::cmutex::CMutex. Verify that thecmutexmodule exists undercrate::bindingand contains theCMutexstruct to ensure the code compiles correctly.
103-105: Addition ofm_mutmethod for mutable accessThe
m_mutmethod provides mutable access to thematrixfield, which enhances flexibility when modifying the matrix. Ensure that mutable access aligns with the intended design and concurrency considerations are addressed.
559-559: Variableashadows previous declaration intest_new_matrixAs above, the variable
ais redeclared at line 559. This shadowing can be confusing.src/graph/matrix/GraphBLAS.rs (2)
365-367: Approved: Enum and attribute additions are correctThe additions of the
#[repr(i32)]and derivations forGrB_Info, as well as new enum variants, are appropriate and correctly implemented. These changes ensure that the Rust enum correctly matches the underlying C representation and derives standard traits for usability.
364-364: Verify the correctness ofGrB_INDEX_MAXvalueThe constant
GrB_INDEX_MAXis defined as:pub const GrB_INDEX_MAX: GrB_Index = (1u64 << 60) - 1;Please confirm that this value accurately reflects the maximum index as defined in the GraphBLAS C API. Ensuring consistency with the original C definition is crucial for correct interoperability.
You can run the following script to compare the value with the GraphBLAS C header files:
This script searches for the definition of
GrB_INDEX_MAXin C header files to ensure consistency.
| @@ -3,4 +3,6 @@ | |||
| * Licensed under the Server Side Public License v1 (SSPLv1). | |||
| */ | |||
|
|
|||
| mod ffi; | |||
There was a problem hiding this comment.
💡 Codebase verification
FFI module defined but not utilized
The ffi.rs file exists and contains FFI-related function declarations. However, no usages of the ffi module were found in the codebase, which may indicate unused or incomplete integration.
🔗 Analysis chain
New FFI module added
The addition of mod ffi; introduces a new module for Foreign Function Interface. This suggests that the graph implementation might involve interactions with code written in other languages.
Let's verify the contents and usage of this new FFI module:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the contents and usage of the new FFI module
# Test 1: Check if the ffi.rs file exists
echo "Checking for ffi.rs file:"
fd --type f "ffi.rs" src/graph
# Test 2: Look for FFI-related function declarations
echo "Searching for FFI-related function declarations:"
rg --type rust "extern \"C\"" src/graph/ffi.rs
# Test 3: Check for usage of the FFI module in other files
echo "Checking for usage of the FFI module:"
rg --type rust "use (crate::)?graph::ffi" src
Length of output: 2659
Script:
#!/bin/bash
# Description: Corrected verification of the new FFI module
# Test 1: Check if the ffi.rs file exists
echo "Checking for ffi.rs file:"
fd --type f "ffi.rs" src/graph
# Test 2: Look for FFI-related function declarations
echo "Searching for FFI-related function declarations in ffi.rs:"
rg --type rust 'extern "C"' src/graph/ffi.rs
# Test 3: Check for usage of the FFI module in other files
echo "Checking for usage of the FFI module:"
rg --type rust 'use (crate::)?graph::ffi' src
Length of output: 3664
src/binding/cmutex.rs
Outdated
| pub fn new() -> Self { | ||
| unsafe { | ||
| let mut mutex = MaybeUninit::uninit(); | ||
| libc::pthread_mutex_init(mutex.as_mut_ptr(), null_mut()); | ||
| Self { | ||
| mutex: mutex.assume_init(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve safety and error handling in the new method.
The new method correctly initializes the mutex, but there are a few improvements that can be made:
- The method should be marked as
unsafedue to the use of unsafe operations. - Error handling should be added for the
pthread_mutex_initcall. - Consider using a const pointer for the mutex attributes instead of
null_mut().
Here's a suggested implementation:
pub unsafe fn new() -> Result<Self, std::io::Error> {
let mut mutex = MaybeUninit::uninit();
let result = libc::pthread_mutex_init(mutex.as_mut_ptr(), std::ptr::null());
if result != 0 {
return Err(std::io::Error::from_raw_os_error(result));
}
Ok(Self {
mutex: mutex.assume_init(),
})
}This implementation:
- Marks the function as
unsafe. - Returns a
Resultto handle potential errors. - Uses
std::ptr::null()instead ofnull_mut()for the attributes pointer. - Checks the return value of
pthread_mutex_initand returns an error if initialization fails.
src/binding/cmutex.rs
Outdated
| pub fn lock(&mut self) { | ||
| unsafe { | ||
| libc::pthread_mutex_lock(&mut self.mutex); | ||
| } | ||
| } | ||
|
|
||
| pub fn unlock(&mut self) { | ||
| unsafe { | ||
| libc::pthread_mutex_unlock(&mut self.mutex); | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve safety and error handling in lock and unlock methods.
The lock and unlock methods need several improvements:
- Both methods should be marked as
unsafedue to the use of unsafe C functions. - Error handling should be added for both
pthread_mutex_lockandpthread_mutex_unlockcalls. - Consider changing the methods to take
&selfinstead of&mut selfto allow multiple locks, which is typical for mutex behavior.
Here's a suggested implementation for both methods:
pub unsafe fn lock(&self) -> Result<(), std::io::Error> {
let result = libc::pthread_mutex_lock(self.mutex.as_ptr());
if result != 0 {
return Err(std::io::Error::from_raw_os_error(result));
}
Ok(())
}
pub unsafe fn unlock(&self) -> Result<(), std::io::Error> {
let result = libc::pthread_mutex_unlock(self.mutex.as_ptr());
if result != 0 {
return Err(std::io::Error::from_raw_os_error(result));
}
Ok(())
}These implementations:
- Mark the functions as
unsafe. - Return a
Resultto handle potential errors. - Take
&selfinstead of&mut self. - Check the return value of the pthread functions and return an error if the operation fails.
src/binding/crwlock.rs
Outdated
| pub fn new() -> Self { | ||
| let mut res = CRWLock { | ||
| rwlock: Box::new(PTHREAD_RWLOCK_INITIALIZER), | ||
| }; | ||
| unsafe { | ||
| libc::pthread_rwlock_init(res.rwlock.as_mut(), null_mut()); | ||
| } | ||
| res | ||
| } |
There was a problem hiding this comment.
Add error handling for pthread_rwlock_init.
The initialization of the rwlock looks correct, but there's no error handling for pthread_rwlock_init. This function can fail in certain conditions (e.g., insufficient memory).
Consider adding error handling:
pub fn new() -> Result<Self, std::io::Error> {
let mut rwlock = Box::new(PTHREAD_RWLOCK_INITIALIZER);
let result = unsafe {
libc::pthread_rwlock_init(rwlock.as_mut(), null_mut())
};
if result != 0 {
Err(std::io::Error::last_os_error())
} else {
Ok(CRWLock { rwlock })
}
}This change would propagate any initialization errors to the caller.
src/binding/crwlock.rs
Outdated
| pub fn acquire_write(&mut self) { | ||
| unsafe { | ||
| let res = libc::pthread_rwlock_wrlock(self.rwlock.as_mut()); | ||
| debug_assert!(res == 0, "pthread_rwlock_wrlock failed"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling for acquire_write.
Similar to acquire_read, this method should handle errors properly and propagate them to the caller. The use of debug_assert! only checks in debug builds.
Suggested improvement:
pub fn acquire_write(&mut self) -> Result<(), std::io::Error> {
let result = unsafe { libc::pthread_rwlock_wrlock(self.rwlock.as_mut()) };
if result != 0 {
Err(std::io::Error::from_raw_os_error(result))
} else {
Ok(())
}
}This change ensures that any errors during lock acquisition are properly handled and communicated to the caller.
src/graph/graph.rs
Outdated
| let m = self.get_node_label_matrix(); | ||
|
|
||
| let mut it = DeltaMatrixIter::new_range(m, n.id, n.id); | ||
| let mut i = 0; | ||
|
|
||
| while let Ok(Some((_, j))) = it.next_bool() { | ||
| labels[i as usize] = j as i32; | ||
| i += 1; | ||
| } | ||
|
|
||
| i | ||
| } |
There was a problem hiding this comment.
Prevent potential buffer overflow in get_node_labels
In the get_node_labels function, there is no check to ensure that the labels slice has sufficient capacity to store all labels of the node. If a node has more labels than the length of the provided labels slice, this could lead to an out-of-bounds write.
Consider modifying the function to return a Vec<i32> containing the labels, or ensure that the labels slice is sized appropriately before calling the function.
src/graph/graph.rs
Outdated
| fn optimized_single_edge_form_connection( | ||
| &mut self, | ||
| src: u64, | ||
| dest: u64, | ||
| edge_id: u64, | ||
| r: i32, | ||
| ) { | ||
| let m = self.get_relation_matrix(r, false); | ||
| m.m_mut().set_element_u64(edge_id, src, dest); | ||
| m.transposed_mut() | ||
| .unwrap() | ||
| .m_mut() | ||
| .set_element_bool(true, dest, src); | ||
|
|
||
| let adj = self.get_adjacency_matrix(false); | ||
| adj.m_mut().set_element_bool(true, src, dest); | ||
| adj.transposed_mut() | ||
| .unwrap() | ||
| .m_mut() | ||
| .set_element_bool(true, dest, src); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code in optimized_single_edge_form_connection
The method optimized_single_edge_form_connection contains duplicated code for setting elements in matrices and their transposes. Refactoring this code can reduce duplication and improve maintainability.
Consider extracting the repeated logic into a helper function to avoid duplication:
fn set_matrix_elements(
matrix: &mut DeltaMatrix,
src: u64,
dest: u64,
value: impl Into<GrB_Value>,
) {
matrix.m_mut().set_element(value.into(), src, dest);
matrix
.transposed_mut()
.unwrap()
.m_mut()
.set_element(value.into(), dest, src);
}
// Then use the helper function:
set_matrix_elements(&mut self.relations[r as usize], src, dest, edge_id);
set_matrix_elements(&mut self.adjacency_matrix, src, dest, true);| unsafe { GrB_Vector_new(v.as_mut_ptr(), GrB_BOOL, node_count) }; | ||
|
|
||
| let mut v = unsafe { v.assume_init() }; |
There was a problem hiding this comment.
Check return value of GrB_Vector_new to handle potential errors
In the set_node_labels function, the call to GrB_Vector_new does not check for errors. Since this function can fail, not checking the return value may lead to undefined behavior.
Modify the code to check the return value and handle errors appropriately:
unsafe {
- GrB_Vector_new(v.as_mut_ptr(), GrB_BOOL, node_count);
+ let info = GrB_Vector_new(v.as_mut_ptr(), GrB_BOOL, node_count);
+ grb_check!(info);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| unsafe { GrB_Vector_new(v.as_mut_ptr(), GrB_BOOL, node_count) }; | |
| let mut v = unsafe { v.assume_init() }; | |
| unsafe { | |
| let info = GrB_Vector_new(v.as_mut_ptr(), GrB_BOOL, node_count); | |
| grb_check!(info); | |
| } | |
| let mut v = unsafe { v.assume_init() }; |
src/graph/graph.rs
Outdated
| debug_assert!({ | ||
| let mut v = Vec::new(); | ||
| nodes.iter().all(|n| { | ||
| self.get_node_edges(n, GraphEdgeDir::Both, -1, &mut v); | ||
| v.is_empty() | ||
| }) | ||
| }); |
There was a problem hiding this comment.
Fix accumulation issue in debug_assert! by clearing vector v
In the delete_nodes function, within the debug_assert!, the vector v is not cleared between iterations. This causes v to accumulate edges from previous nodes, leading to incorrect assertion behavior when checking v.is_empty().
Apply this diff to clear v at the beginning of each iteration:
debug_assert!({
let mut v = Vec::new();
nodes.iter().all(|n| {
+ v.clear();
self.get_node_edges(n, GraphEdgeDir::Both, -1, &mut v);
v.is_empty()
})
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| debug_assert!({ | |
| let mut v = Vec::new(); | |
| nodes.iter().all(|n| { | |
| self.get_node_edges(n, GraphEdgeDir::Both, -1, &mut v); | |
| v.is_empty() | |
| }) | |
| }); | |
| debug_assert!({ | |
| let mut v = Vec::new(); | |
| nodes.iter().all(|n| { | |
| v.clear(); | |
| self.get_node_edges(n, GraphEdgeDir::Both, -1, &mut v); | |
| v.is_empty() | |
| }) | |
| }); |
src/graph/graph.rs
Outdated
| pub fn new( | ||
| node_cap: u64, | ||
| edge_cap: u64, | ||
| ) -> Self { | ||
| let nodes = unsafe { | ||
| DataBlock_New( | ||
| node_cap, | ||
| node_cap, | ||
| size_of::<AttributeSet>() as u32, | ||
| transmute_copy(&(AttributeSet_Free as *const c_void)), | ||
| ) | ||
| }; | ||
| let edges = unsafe { | ||
| DataBlock_New( | ||
| edge_cap, | ||
| edge_cap, | ||
| size_of::<AttributeSet>() as u32, | ||
| transmute_copy(&(AttributeSet_Free as *const c_void)), | ||
| ) | ||
| }; | ||
| Self { | ||
| reserved_node_count: 0, | ||
| nodes: nodes, | ||
| edges: edges, | ||
| adjacency_matrix: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, true), | ||
| labels: Vec::new(), | ||
| node_labels: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, false), | ||
| relations: Vec::new(), | ||
| zero_matrix: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, false), | ||
| rwlock: CRWLock::new(), | ||
| writelocked: false, | ||
| matrix_policy: MatrixPolicy::FlushResize, | ||
| stats: GraphStatistics { | ||
| node_count: Vec::new(), | ||
| edge_count: Vec::new(), | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle potential null pointers in Graph::new
In the Graph::new function, the calls to DataBlock_New are unsafe and may return null pointers if the allocation fails. Dereferencing these null pointers can lead to undefined behavior.
Add checks to ensure that nodes and edges are not null before proceeding:
let nodes = unsafe { DataBlock_New(...) };
+assert!(!nodes.is_null(), "Failed to allocate nodes DataBlock");
let edges = unsafe { DataBlock_New(...) };
+assert!(!edges.is_null(), "Failed to allocate edges DataBlock");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn new( | |
| node_cap: u64, | |
| edge_cap: u64, | |
| ) -> Self { | |
| let nodes = unsafe { | |
| DataBlock_New( | |
| node_cap, | |
| node_cap, | |
| size_of::<AttributeSet>() as u32, | |
| transmute_copy(&(AttributeSet_Free as *const c_void)), | |
| ) | |
| }; | |
| let edges = unsafe { | |
| DataBlock_New( | |
| edge_cap, | |
| edge_cap, | |
| size_of::<AttributeSet>() as u32, | |
| transmute_copy(&(AttributeSet_Free as *const c_void)), | |
| ) | |
| }; | |
| Self { | |
| reserved_node_count: 0, | |
| nodes: nodes, | |
| edges: edges, | |
| adjacency_matrix: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, true), | |
| labels: Vec::new(), | |
| node_labels: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, false), | |
| relations: Vec::new(), | |
| zero_matrix: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, false), | |
| rwlock: CRWLock::new(), | |
| writelocked: false, | |
| matrix_policy: MatrixPolicy::FlushResize, | |
| stats: GraphStatistics { | |
| node_count: Vec::new(), | |
| edge_count: Vec::new(), | |
| }, | |
| } | |
| } | |
| pub fn new( | |
| node_cap: u64, | |
| edge_cap: u64, | |
| ) -> Self { | |
| let nodes = unsafe { | |
| DataBlock_New( | |
| node_cap, | |
| node_cap, | |
| size_of::<AttributeSet>() as u32, | |
| transmute_copy(&(AttributeSet_Free as *const c_void)), | |
| ) | |
| }; | |
| assert!(!nodes.is_null(), "Failed to allocate nodes DataBlock"); | |
| let edges = unsafe { | |
| DataBlock_New( | |
| edge_cap, | |
| edge_cap, | |
| size_of::<AttributeSet>() as u32, | |
| transmute_copy(&(AttributeSet_Free as *const c_void)), | |
| ) | |
| }; | |
| assert!(!edges.is_null(), "Failed to allocate edges DataBlock"); | |
| Self { | |
| reserved_node_count: 0, | |
| nodes: nodes, | |
| edges: edges, | |
| adjacency_matrix: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, true), | |
| labels: Vec::new(), | |
| node_labels: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, false), | |
| relations: Vec::new(), | |
| zero_matrix: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, false), | |
| rwlock: CRWLock::new(), | |
| writelocked: false, | |
| matrix_policy: MatrixPolicy::FlushResize, | |
| stats: GraphStatistics { | |
| node_count: Vec::new(), | |
| edge_count: Vec::new(), | |
| }, | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (4)
src/graph/graph.rs (4)
102-116: Add documentation for theGraphstruct and its fieldsThe
Graphstruct is a central part of this module, but it lacks documentation. Adding clear and concise documentation for the struct and its fields would greatly improve code readability and maintainability.Consider adding documentation comments to the
Graphstruct and its fields. For example:/// Represents a graph data structure with nodes, edges, and their relationships. pub struct Graph { /// The number of reserved nodes. reserved_node_count: i32, /// Pointer to the data block containing nodes. nodes: *mut DataBlock, /// Pointer to the data block containing edges. edges: *mut DataBlock, /// The adjacency matrix representing connections between nodes. adjacency_matrix: DeltaMatrix, /// Vector of matrices representing node labels. labels: Vec<DeltaMatrix>, /// Matrix representing the labels of each node. node_labels: DeltaMatrix, /// Vector of tensors representing relationships between nodes. relations: Vec<Tensor>, // ... add comments for other fields }Adding these comments will help other developers (including your future self) understand the purpose and usage of each field in the
Graphstruct.
1070-1240: Consider a safer approach for test initializationThe current test initialization uses unsafe code to reset allocations and set global options. This could lead to undefined behavior if not used correctly.
Consider creating safe wrapper functions for the initialization steps:
fn safe_init() -> Result<(), &'static str> { unsafe { Alloc_Reset(); if GrB_init(GrB_Mode::GrB_NONBLOCKING) != GrB_Info::GrB_SUCCESS { return Err("Failed to initialize GraphBLAS"); } if GxB_Global_Option_set(GxB_Option_Field::GxB_FORMAT, GxB_Format_Value::GxB_BY_ROW) != GrB_Info::GrB_SUCCESS { return Err("Failed to set global option"); } if Config_Option_set( ConfigOptionField::DELTA_MAX_PENDING_CHANGES, "10000\0".as_ptr() as *const c_char, null_mut(), ) != 0 { return Err("Failed to set config option"); } } Ok(()) } // Then in each test: #[test] fn test_new_graph() -> Result<(), &'static str> { safe_init()?; // ... rest of the test ... Ok(()) }This approach provides a safer interface for test initialization and allows for proper error handling.
1070-1240: Add tests for error casesThe current test suite focuses on successful operations but doesn't cover error cases. Adding tests for error scenarios would improve the robustness of the code.
Consider adding tests for error cases, such as:
- Creating a graph with invalid dimensions
- Adding an edge between non-existent nodes
- Deleting a node that has edges
- Accessing a non-existent node or edge
For example:
#[test] fn test_create_edge_with_invalid_nodes() { let mut g = Graph::new(5, 5); let mut edge = Edge::default(); assert!(g.create_edge(10, 11, 0, &mut edge).is_err()); } #[test] fn test_delete_node_with_edges() { let mut g = Graph::new(5, 5); let mut n1 = g.reserve_node(); let mut n2 = g.reserve_node(); g.create_node(&mut n1, &[]); g.create_node(&mut n2, &[]); let r = g.add_relation_type(); let mut edge = Edge::default(); g.create_edge(n1.id, n2.id, r, &mut edge); assert!(g.delete_nodes(&[n1]).is_err()); }These additional tests will help ensure that the
Graphstruct handles error cases correctly.
1070-1240: Add tests for uncovered public methodsSome public methods of the
Graphstruct are not covered by the current test suite. Adding tests for these methods would improve code coverage and ensure their correct functionality.Consider adding tests for the following methods:
get_node_labelsget_node_degreeget_edges_connecting_nodesset_node_labelsrelationship_contains_multi_edgeFor example:
#[test] fn test_get_node_labels() { let mut g = Graph::new(5, 5); let mut node = g.reserve_node(); let label1 = g.add_label(); let label2 = g.add_label(); g.create_node(&mut node, &[label1, label2]); let mut labels = vec![0; 2]; let count = g.get_node_labels(&node, &mut labels); assert_eq!(count, 2); assert_eq!(labels, vec![label1, label2]); } #[test] fn test_get_node_degree() { let mut g = Graph::new(5, 5); let mut n1 = g.reserve_node(); let mut n2 = g.reserve_node(); g.create_node(&mut n1, &[]); g.create_node(&mut n2, &[]); let r = g.add_relation_type(); let mut edge = Edge::default(); g.create_edge(n1.id, n2.id, r, &mut edge); assert_eq!(g.get_node_degree(&n1, GraphEdgeDir::Outgoing, -1), 1); assert_eq!(g.get_node_degree(&n2, GraphEdgeDir::Incoming, -1), 1); }These additional tests will help ensure that all public methods of the
Graphstruct are working correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/binding/graph.rs (4 hunks)
- src/graph/ffi.rs (1 hunks)
- src/graph/graph.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/binding/graph.rs
- src/graph/ffi.rs
| pub fn new( | ||
| node_cap: u64, | ||
| edge_cap: u64, | ||
| ) -> Self { | ||
| let nodes = unsafe { | ||
| DataBlock_New( | ||
| node_cap, | ||
| node_cap, | ||
| size_of::<AttributeSet>() as u32, | ||
| transmute_copy(&(AttributeSet_Free as *const c_void)), | ||
| ) | ||
| }; |
There was a problem hiding this comment.
Add error handling for DataBlock_New calls
The Graph::new method uses unsafe calls to DataBlock_New without checking if the returned pointers are null. This could lead to undefined behavior if the allocation fails.
Consider adding null checks and error handling for the DataBlock_New calls:
let nodes = unsafe {
let ptr = DataBlock_New(
node_cap,
node_cap,
size_of::<AttributeSet>() as u32,
transmute_copy(&(AttributeSet_Free as *const c_void)),
);
if ptr.is_null() {
// Handle error, e.g., return a Result instead of Self
panic!("Failed to allocate nodes DataBlock");
}
ptr
};Apply similar changes to the edges allocation as well.
src/graph/graph.rs
Outdated
| Self { | ||
| reserved_node_count: 0, | ||
| nodes: nodes, | ||
| edges: edges, | ||
| adjacency_matrix: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, true), | ||
| labels: Vec::new(), | ||
| node_labels: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, false), | ||
| relations: Vec::new(), | ||
| zero_matrix: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, false), | ||
| rwlock: CRWLock::new(), | ||
| writelocked: false, | ||
| matrix_policy: MatrixPolicy::FlushResize, | ||
| stats: GraphStatistics { | ||
| node_count: Vec::new(), | ||
| edge_count: Vec::new(), | ||
| }, | ||
| partial: false, | ||
| } |
There was a problem hiding this comment.
Check for potential integer overflow in matrix dimensions
When initializing matrices in the Graph::new method, there's a risk of integer overflow if node_cap is very large. This could lead to unexpected behavior or panics.
Consider adding checks to ensure that node_cap doesn't exceed the maximum allowed dimension for your matrix implementations:
fn new(node_cap: u64, edge_cap: u64) -> Result<Self, &'static str> {
if node_cap > some_max_dimension {
return Err("Node capacity exceeds maximum allowed dimension");
}
// ... rest of the initialization code ...
Ok(Self {
// ... struct fields ...
})
}This change will help prevent potential issues with very large graph initializations.
| if id != INVALID_ENTITY_ID { | ||
| debug_assert!(id == n.id); | ||
| self.reserved_node_count -= 1; | ||
| debug_assert!(self.reserved_node_count >= 0); |
There was a problem hiding this comment.
Prevent potential integer underflow when decrementing reserved_node_count
When decrementing reserved_node_count, there's no check to ensure it doesn't underflow. This could lead to unexpected behavior if reserved_node_count is already 0.
Add a check to prevent underflow:
if id != INVALID_ENTITY_ID {
debug_assert!(id == n.id);
if self.reserved_node_count > 0 {
self.reserved_node_count -= 1;
} else {
// Log a warning or handle the unexpected state
}
debug_assert!(self.reserved_node_count >= 0);
}This change will help prevent potential issues with unexpected decrements of reserved_node_count.
| debug_assert!(self.get_node(src).is_some()); | ||
| debug_assert!(self.get_node(dest).is_some()); | ||
| debug_assert!(r >= 0 && r < self.relations.len() as i32); | ||
|
|
There was a problem hiding this comment.
Replace debug assertions with proper validation in create_edge
The create_edge method uses debug_assert! for validating input parameters. However, these assertions are only checked in debug builds and are completely removed in release builds, potentially leading to undefined behavior if invalid inputs are provided.
Replace the debug assertions with proper validation that's active in all build configurations:
pub fn create_edge(&mut self, src: NodeID, dest: NodeID, r: RelationID, e: &mut Edge) -> Result<(), &'static str> {
if self.get_node(src).is_none() {
return Err("Source node does not exist");
}
if self.get_node(dest).is_none() {
return Err("Destination node does not exist");
}
if r < 0 || r >= self.relations.len() as i32 {
return Err("Invalid relation ID");
}
// ... rest of the method ...
}This change ensures that invalid inputs are caught and handled appropriately in all build configurations.
src/graph/graph.rs
Outdated
| pub fn delete_nodes( | ||
| &mut self, | ||
| nodes: &[Node], | ||
| ) { | ||
| let policy = self.set_matrix_policy(MatrixPolicy::Nop); | ||
|
|
||
| debug_assert!({ | ||
| let mut v = Vec::new(); | ||
| nodes.iter().all(|n| { | ||
| self.get_node_edges(n, GraphEdgeDir::Both, -1, &mut v); | ||
| v.is_empty() | ||
| }) | ||
| }); |
There was a problem hiding this comment.
Improve edge checking efficiency in delete_nodes
The current implementation uses a debug assertion with a nested loop to check if nodes have no edges. This is inefficient and only works in debug builds.
Consider replacing the debug assertion with a more efficient check that works in all build configurations:
pub fn delete_nodes(&mut self, nodes: &[Node]) -> Result<(), &'static str> {
let policy = self.set_matrix_policy(MatrixPolicy::Nop);
for node in nodes {
if self.get_node_degree(node, GraphEdgeDir::Both, -1) > 0 {
return Err("Cannot delete node with existing edges");
}
}
// ... rest of the method ...
Ok(())
}This change improves efficiency and ensures that the check is performed in all build configurations.
src/graph/graph.rs
Outdated
| let mut elems = SparseMatrix::new( | ||
| unsafe { GrB_BOOL }, | ||
| self.required_matrix_dim(), | ||
| self.required_matrix_dim(), | ||
| ); | ||
|
|
||
| for node in nodes { | ||
| let mut it = DeltaMatrixIter::new_range(&self.node_labels, node.id, node.id); | ||
|
|
||
| while let Ok(Some((_, j))) = it.next_bool() { | ||
| elems.set_element_bool(true, node.id, j); | ||
| self.labels[j as usize].remove_element(node.id, node.id); | ||
| self.stats.decrement_node_count(j as LabelID, 1); | ||
| } | ||
| unsafe { DataBlock_DeleteItem(self.nodes, node.id) }; | ||
| } | ||
|
|
||
| self.node_labels.remove_elements(&elems); | ||
|
|
||
| self.set_matrix_policy(policy); |
There was a problem hiding this comment.
Add error handling for matrix operations in delete_nodes
The delete_nodes method performs several matrix operations without checking for potential errors. This could lead to unexpected behavior if any of these operations fail.
Consider adding error handling for matrix operations:
pub fn delete_nodes(&mut self, nodes: &[Node]) -> Result<(), &'static str> {
// ... existing code ...
let mut elems = SparseMatrix::new(
unsafe { GrB_BOOL },
self.required_matrix_dim(),
self.required_matrix_dim(),
);
for node in nodes {
let mut it = DeltaMatrixIter::new_range(&self.node_labels, node.id, node.id);
while let Ok(Some((_, j))) = it.next_bool() {
elems.set_element_bool(true, node.id, j).map_err(|_| "Failed to set element in SparseMatrix")?;
self.labels[j as usize].remove_element(node.id, node.id).map_err(|_| "Failed to remove element from label matrix")?;
self.stats.decrement_node_count(j as LabelID, 1);
}
unsafe { DataBlock_DeleteItem(self.nodes, node.id) };
}
self.node_labels.remove_elements(&elems).map_err(|_| "Failed to remove elements from node_labels matrix")?;
self.set_matrix_policy(policy);
Ok(())
}This change ensures that any errors during matrix operations are properly handled and reported.
| for e in edges.iter() { | ||
| unsafe { DataBlock_DeleteItem(self.edges, e.id) }; | ||
| } |
There was a problem hiding this comment.
Consider a safer approach for deleting items from the DataBlock
The current implementation uses unsafe code to delete items from the DataBlock. This could lead to undefined behavior if not used correctly.
Consider creating a safe wrapper method for DataBlock_DeleteItem that handles potential errors:
impl Graph {
// ... other methods ...
fn safe_delete_edge(&self, edge_id: u64) -> Result<(), &'static str> {
let result = unsafe { DataBlock_DeleteItem(self.edges, edge_id) };
if result != 0 { // Assuming 0 indicates success, adjust if needed
Err("Failed to delete edge from DataBlock")
} else {
Ok(())
}
}
}
// Then in delete_edges:
pub fn delete_edges(&mut self, edges: &mut [Edge]) -> Result<(), &'static str> {
// ... other code ...
for e in edges.iter() {
self.safe_delete_edge(e.id)?;
}
// ... rest of the method ...
}This approach provides a safer interface for deleting edges and allows for proper error handling.
There was a problem hiding this comment.
Actionable comments posted: 15
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/graph/ffi.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
src/graph/ffi.rs (2)
1-33: LGTM: File header and imports are appropriate.The file header includes proper copyright and license information. The imports are relevant for the FFI implementation, including necessary types and modules from both the standard library and the crate.
484-545: Improve safety in graph modification functions.The functions Graph_EnsureNodeCap, Graph_MarkEdgeDeleted, Graph_MarkNodeDeleted, Graph_SetNode, Graph_SetNodeLabels, and Graph_SetEdge correctly wrap the corresponding methods of the Graph struct. However, they lack null pointer checks and have potential safety issues with raw pointer usage.
- Add null pointer checks for all input pointers.
- Use safer alternatives to unwrap() when dealing with potentially null pointers.
- Consider returning a boolean to indicate success or failure of the operation.
- In Graph_SetNode, use a safer method for creating a slice from raw parts.
Example improvements:
unsafe extern "C" fn Graph_EnsureNode </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_GetMatrixPolicy(g: *mut Graph) -> MatrixPolicy { | ||
| (&*g).get_matrix_policy() | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_SetMatrixPolicy( | ||
| g: *mut Graph, | ||
| policy: MatrixPolicy, | ||
| ) -> MatrixPolicy { | ||
| (&mut *g).set_matrix_policy(policy) | ||
| } |
There was a problem hiding this comment.
Add null pointer checks for Graph_GetMatrixPolicy and Graph_SetMatrixPolicy.
While these functions correctly wrap the corresponding methods of the Graph struct, they lack null pointer checks on the graph pointer. This could lead to undefined behavior if a null pointer is passed.
Consider adding null pointer checks at the beginning of these functions:
unsafe extern "C" fn Graph_GetMatrixPolicy(g: *mut Graph) -> MatrixPolicy {
if g.is_null() {
return MatrixPolicy::default(); // Or some error indicator
}
(&*g).get_matrix_policy()
}
unsafe extern "C" fn Graph_SetMatrixPolicy(
g: *mut Graph,
policy: MatrixPolicy,
) -> MatrixPolicy {
if g.is_null() {
return MatrixPolicy::default(); // Or some error indicator
}
(&mut *g).set_matrix_policy(policy)
}This change will prevent potential crashes or undefined behavior when these functions are called with a null pointer.
src/graph/ffi.rs
Outdated
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_RelationshipContainsMultiEdge( | ||
| g: *mut Graph, | ||
| r: RelationID, | ||
| ) -> bool { | ||
| (&mut *g).relationship_contains_multi_edge(r) | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_GetNode( | ||
| g: *mut Graph, | ||
| id: NodeID, | ||
| n: *mut Node, | ||
| ) -> bool { | ||
| match (&*g).get_node(id) { | ||
| Some(node) => { | ||
| n.write(node); | ||
| true | ||
| } | ||
| None => false, | ||
| } | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_GetEdge( | ||
| g: *mut Graph, | ||
| id: EdgeID, | ||
| e: *mut Edge, | ||
| ) -> bool { | ||
| (&*g).get_edge(id, e.as_mut().unwrap()) | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_GetEdgesConnectingNodes( | ||
| g: *mut Graph, | ||
| src_id: NodeID, | ||
| dest_id: NodeID, | ||
| r: RelationID, | ||
| edges: *mut *mut Edge, | ||
| ) { | ||
| let es = (&mut *g).get_edges_connecting_nodes(src_id, dest_id, r); | ||
| let mut arr_ptr = (*edges as *mut ArrayHeader).sub(1); | ||
| let mut arr = arr_ptr.as_mut().unwrap(); | ||
| if arr.cap - arr.len < es.len() as u32 { | ||
| arr.cap = arr.len + es.len() as u32; | ||
| arr_ptr = RedisModule_Realloc.unwrap()( | ||
| arr_ptr as _, | ||
| (arr.cap as usize * size_of::<Edge>() + size_of::<ArrayHeader>()) as usize, | ||
| ) as _; | ||
| edges.write(arr_ptr.add(1) as _); | ||
| arr = arr_ptr.as_mut().unwrap(); | ||
| } | ||
| for (i, e) in es.iter().enumerate() { | ||
| (*edges).add(arr.len as usize + i).write(*e); | ||
| } | ||
| arr.len += es.len() as u32; | ||
| } |
There was a problem hiding this comment.
Improve safety and add parameter validation in graph operation functions.
The functions Graph_RelationshipContainsMultiEdge, Graph_GetNode, Graph_GetEdge, and Graph_GetEdgesConnectingNodes correctly wrap the corresponding methods of the Graph struct. However, there are potential safety issues with null pointer dereferencing and a lack of checks for some input parameters.
- Add null pointer checks for all input pointers.
- Validate input parameters where applicable.
- Use safer alternatives to unwrap() when dealing with potentially null pointers.
- Consider returning a boolean or error code to indicate success or failure of the operation.
Example improvements:
unsafe extern "C" fn Graph_RelationshipContainsMultiEdge(
g: *mut Graph,
r: RelationID,
) -> bool {
if g.is_null() {
return false;
}
(&mut *g).relationship_contains_multi_edge(r)
}
unsafe extern "C" fn Graph_GetNode(
g: *mut Graph,
id: NodeID,
n: *mut Node,
) -> bool {
if g.is_null() || n.is_null() {
return false;
}
match (&*g).get_node(id) {
Some(node) => {
*n = node;
true
}
None => false,
}
}
unsafe extern "C" fn Graph_GetEdge(
g: *mut Graph,
id: EdgeID,
e: *mut Edge,
) -> bool {
if g.is_null() || e.is_null() {
return false;
}
(&*g).get_edge(id, &mut *e)
}
unsafe extern "C" fn Graph_GetEdgesConnectingNodes(
g: *mut Graph,
src_id: NodeID,
dest_id: NodeID,
r: RelationID,
edges: *mut *mut Edge,
) -> bool {
if g.is_null() || edges.is_null() {
return false;
}
// ... (rest of the function with additional safety checks)
}These changes improve safety by checking for null pointers, validating input parameters, and providing a way for the caller to know if the operation was successful.
src/graph/ffi.rs
Outdated
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_GetNodeEdges( | ||
| g: *mut Graph, | ||
| n: *const Node, | ||
| dir: GraphEdgeDir, | ||
| edge_type: RelationID, | ||
| edges: *mut *mut Edge, | ||
| ) { | ||
| let mut es = Vec::new(); | ||
| (&mut *g).get_node_edges(n.as_ref().unwrap(), dir, edge_type, &mut es); | ||
| let mut arr_ptr = (*edges as *mut ArrayHeader).sub(1); | ||
| let mut arr = arr_ptr.as_mut().unwrap(); | ||
| if arr.cap - arr.len < es.len() as u32 { | ||
| arr.cap = arr.len + es.len() as u32; | ||
| arr_ptr = RedisModule_Realloc.unwrap()( | ||
| arr_ptr as _, | ||
| (arr.cap as usize * size_of::<Edge>() + size_of::<ArrayHeader>()) as usize, | ||
| ) as _; | ||
| edges.write(arr_ptr.add(1) as _); | ||
| arr = arr_ptr.as_mut().unwrap(); | ||
| } | ||
| for (i, e) in es.iter().enumerate() { | ||
| (*edges).add(arr.len as usize + i).write(*e); | ||
| } | ||
| arr.len += es.len() as u32; | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_GetNodeDegree( | ||
| g: *mut Graph, | ||
| n: *const Node, | ||
| dir: GraphEdgeDir, | ||
| edge_type: RelationID, | ||
| ) -> u64 { | ||
| (&mut *g).get_node_degree(n.as_ref().unwrap(), dir, edge_type) | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_GetNodeLabels( | ||
| g: *mut Graph, | ||
| n: *const Node, | ||
| labels: *mut LabelID, | ||
| label_count: u32, | ||
| ) -> u32 { | ||
| (&mut *g).get_node_labels( | ||
| n.as_ref().unwrap(), | ||
| from_raw_parts_mut(labels, label_count as usize), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Enhance safety in node information retrieval functions.
The functions Graph_GetNodeEdges, Graph_GetNodeDegree, and Graph_GetNodeLabels correctly wrap the corresponding methods of the Graph struct. However, there are potential safety issues with null pointer dereferencing and the use of from_raw_parts.
- Add null pointer checks for all input pointers.
- Use safer alternatives to unwrap() when dealing with potentially null pointers.
- Consider returning a boolean or error code to indicate success or failure of the operation.
- In Graph_GetNodeEdges, consider using a safer method for array resizing.
Example improvements:
unsafe extern "C" fn Graph_GetNodeEdges(
g: *mut Graph,
n: *const Node,
dir: GraphEdgeDir,
edge_type: RelationID,
edges: *mut *mut Edge,
) -> bool {
if g.is_null() || n.is_null() || edges.is_null() {
return false;
}
let mut es = Vec::new();
(&mut *g).get_node_edges(n.as_ref().unwrap(), dir, edge_type, &mut es);
// ... (implement safe array resizing logic here)
true
}
unsafe extern "C" fn Graph_GetNodeDegree(
g: *mut Graph,
n: *const Node,
dir: GraphEdgeDir,
edge_type: RelationID,
) -> u64 {
if g.is_null() || n.is_null() {
return 0;
}
(&mut *g).get_node_degree(n.as_ref().unwrap(), dir, edge_type)
}
unsafe extern "C" fn Graph_GetNodeLabels(
g: *mut Graph,
n: *const Node,
labels: *mut LabelID,
label_count: u32,
) -> u32 {
if g.is_null() || n.is_null() || labels.is_null() {
return 0;
}
(&mut *g).get_node_labels(
n.as_ref().unwrap(),
std::slice::from_raw_parts_mut(labels, label_count as usize),
)
}These changes improve safety by checking for null pointers and provide a way for the caller to know if the operation was successful.
…core-rs into rewrite-graph
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
src/graph/graph.rs (4)
39-46: Consider adding documentation forMatrixPolicyenumThe
MatrixPolicyenum is newly added, but it lacks documentation explaining the purpose and use cases for each variant. Adding documentation would improve code readability and maintainability.Consider adding documentation comments for the
MatrixPolicyenum and its variants:/// Defines the policy for matrix operations in the graph. #[repr(C)] #[allow(dead_code)] #[derive(Clone, Copy)] pub enum MatrixPolicy { /// Flush and resize the matrix. FlushResize, /// Only resize the matrix without flushing. Resize, /// No operation on the matrix. Nop, }
48-54: Add documentation forGraphEdgeDirenumThe
GraphEdgeDirenum is newly added but lacks documentation. Adding documentation would improve code readability and help users understand the purpose of each variant.Consider adding documentation comments for the
GraphEdgeDirenum and its variants:/// Defines the direction of edges in the graph. #[repr(C)] #[derive(PartialEq, Eq)] pub enum GraphEdgeDir { /// Incoming edges. Incoming, /// Outgoing edges. Outgoing, /// Both incoming and outgoing edges. Both, }
1069-1240: Enhance unit tests for better coverage and error handlingWhile the existing unit tests provide good coverage, there are opportunities to improve their robustness and cover more edge cases.
Consider the following improvements to the unit tests:
- Add tests for error cases, such as creating a graph with zero capacity or allocating nodes/edges beyond capacity.
- Test the
GraphStatisticsmethods to ensure they handle edge cases correctly.- Add tests for the
MatrixPolicyandGraphEdgeDirenums to ensure they behave as expected.- Include tests for concurrent access to the graph using multiple threads.
- Test the
Dropimplementation to ensure all resources are properly freed.Example of an additional test:
#[test] fn test_graph_capacity_limits() { test_init(); let mut g = Graph::new(5, 5); g.acquire_write_lock(); // Test node capacity for _ in 0..5 { let mut n = g.reserve_node(); g.create_node(&mut n, &[]).expect("Should create node successfully"); } let mut n = g.reserve_node(); assert!(g.create_node(&mut n, &[]).is_err(), "Should fail to create node beyond capacity"); // Test edge capacity let r = g.add_relation_type(); let mut edge = Edge::default(); for i in 0..4 { g.create_edge(0, i + 1, r, &mut edge).expect("Should create edge successfully"); } assert!(g.create_edge(0, 4, r, &mut edge).is_err(), "Should fail to create edge beyond capacity"); g.release_lock(); }These improvements will help ensure that the Graph implementation handles various scenarios correctly and remains robust under different conditions.
1-1240: Overall assessment: Robust implementation with room for improvementThe Graph implementation in
src/graph/graph.rsis comprehensive and covers a wide range of functionality. However, there are several areas where the code could be improved:
- Error handling: Many methods could benefit from better error handling, particularly when dealing with unsafe operations and FFI calls.
- Safety: The use of unsafe code, while necessary for FFI, could be better documented and contained.
- Documentation: Adding more documentation, especially for public APIs and unsafe operations, would improve code readability and maintainability.
- Unit tests: While the existing tests provide good coverage, there's room for improvement in testing edge cases and error scenarios.
Consider addressing these points to make the implementation more robust, safer, and easier to maintain. Additionally, it may be worth exploring ways to reduce the use of unsafe code where possible, perhaps by wrapping unsafe operations in safe abstractions.
| let nodes = unsafe { | ||
| DataBlock_New( | ||
| node_cap, | ||
| node_cap, | ||
| size_of::<AttributeSet>() as u32, | ||
| transmute_copy(&(AttributeSet_Free as *const c_void)), | ||
| ) | ||
| }; |
There was a problem hiding this comment.
Improve safety and documentation for unsafe FFI calls in Graph::new
The Graph::new method contains unsafe FFI calls to DataBlock_New without proper documentation or error handling.
Consider adding documentation and improving error handling for these unsafe calls:
pub fn new(node_cap: u64, edge_cap: u64) -> Result<Self, &'static str> {
// Allocate nodes DataBlock
let nodes = unsafe {
// SAFETY: DataBlock_New is an FFI call that allocates memory.
// It's unsafe because it can return a null pointer if allocation fails.
let ptr = DataBlock_New(
node_cap,
node_cap,
size_of::<AttributeSet>() as u32,
transmute_copy(&(AttributeSet_Free as *const c_void)),
);
if ptr.is_null() {
return Err("Failed to allocate nodes DataBlock");
}
ptr
};
// Allocate edges DataBlock
let edges = unsafe {
// SAFETY: DataBlock_New is an FFI call that allocates memory.
// It's unsafe because it can return a null pointer if allocation fails.
let ptr = DataBlock_New(
edge_cap,
edge_cap,
size_of::<AttributeSet>() as u32,
transmute_copy(&(AttributeSet_Free as *const c_void)),
);
if ptr.is_null() {
// Free the previously allocated nodes DataBlock
DataBlock_Free(nodes);
return Err("Failed to allocate edges DataBlock");
}
ptr
};
// ... rest of the method ...
}This change improves safety by handling allocation failures and adds documentation to explain why unsafe code is necessary.
Also applies to: 131-138
| unsafe { | ||
| let it = if self.partial { | ||
| DataBlock_FullScan(self.nodes) | ||
| } else { | ||
| self.scan_nodes() | ||
| }; | ||
| loop { | ||
| let set = DataBlockIterator_Next(it, null_mut()) as *mut AttributeSet; | ||
| if set.is_null() { | ||
| break; | ||
| } | ||
| AttributeSet_Free(set); | ||
| } | ||
| DataBlockIterator_Free(it); | ||
|
|
||
| let it = if self.partial { | ||
| DataBlock_FullScan(self.edges) | ||
| } else { | ||
| self.scan_edges() | ||
| }; | ||
| loop { | ||
| let set = DataBlockIterator_Next(it, null_mut()) as *mut AttributeSet; | ||
| if set.is_null() { | ||
| break; | ||
| } | ||
| AttributeSet_Free(set); | ||
| } | ||
| DataBlockIterator_Free(it); | ||
|
|
||
| DataBlock_Free(self.nodes); | ||
| DataBlock_Free(self.edges); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve safety in Drop implementation for Graph
The Drop implementation for Graph contains several unsafe operations without proper documentation or error handling.
Consider adding documentation and improving error handling for these unsafe operations:
impl Drop for Graph {
fn drop(&mut self) {
unsafe {
// SAFETY: DataBlock_FullScan and DataBlock_Scan are FFI calls that return an iterator.
// We need to ensure that we free all resources properly.
let it = if self.partial {
DataBlock_FullScan(self.nodes)
} else {
self.scan_nodes()
};
// Free all node attribute sets
loop {
let set = DataBlockIterator_Next(it, null_mut()) as *mut AttributeSet;
if set.is_null() {
break;
}
// SAFETY: AttributeSet_Free is an FFI call that frees memory.
// We assume that the set pointer is valid as it comes from DataBlockIterator_Next.
AttributeSet_Free(set);
}
DataBlockIterator_Free(it);
// Similar process for edges
// ... (omitted for brevity)
// SAFETY: DataBlock_Free is an FFI call that frees memory.
// We assume that self.nodes and self.edges are valid pointers.
DataBlock_Free(self.nodes);
DataBlock_Free(self.edges);
}
}
}This change adds documentation to explain the safety considerations for each unsafe operation and improves the overall safety of the Drop implementation.
src/graph/ffi.rs
Outdated
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_SetNodeLabels(g: *mut Graph) { | ||
| (&mut *g).set_node_labels(); | ||
| } |
There was a problem hiding this comment.
Add null pointer check in Graph_SetNodeLabels.
Currently, g is dereferenced without checking for null, which can lead to undefined behavior if a null pointer is passed. To enhance safety, add a null pointer check for g.
Apply this diff to add the null pointer check:
unsafe extern "C" fn Graph_SetNodeLabels(g: *mut Graph) {
+ if g.is_null() {
+ return;
+ }
(&mut *g).set_node_labels();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_SetNodeLabels(g: *mut Graph) { | |
| (&mut *g).set_node_labels(); | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_SetNodeLabels(g: *mut Graph) { | |
| if g.is_null() { | |
| return; | |
| } | |
| (&mut *g).set_node_labels(); | |
| } |
src/graph/ffi.rs
Outdated
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_GetDeletedNodesList(g: *mut Graph) -> *mut u64 { | ||
| (&*g).get_deleted_nodes_list() | ||
| } | ||
|
|
There was a problem hiding this comment.
Add null pointer check in Graph_GetDeletedNodesList.
Currently, g is dereferenced without checking for null, which can lead to undefined behavior if a null pointer is passed. To enhance safety, add a null pointer check for g.
Apply this diff to add the null pointer check:
unsafe extern "C" fn Graph_GetDeletedNodesList(g: *mut Graph) -> *mut u64 {
+ if g.is_null() {
+ return std::ptr::null_mut();
+ }
(&*g).get_deleted_nodes_list()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_GetDeletedNodesList(g: *mut Graph) -> *mut u64 { | |
| (&*g).get_deleted_nodes_list() | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_GetDeletedNodesList(g: *mut Graph) -> *mut u64 { | |
| if g.is_null() { | |
| return std::ptr::null_mut(); | |
| } | |
| (&*g).get_deleted_nodes_list() | |
| } |
src/graph/ffi.rs
Outdated
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_GetDeletedEdgesList(g: *mut Graph) -> *mut u64 { | ||
| (&*g).get_deleted_edges_list() | ||
| } | ||
|
|
There was a problem hiding this comment.
Add null pointer check in Graph_GetDeletedEdgesList.
Currently, g is dereferenced without checking for null, which can lead to undefined behavior if a null pointer is passed. To enhance safety, add a null pointer check for g.
Apply this diff to add the null pointer check:
unsafe extern "C" fn Graph_GetDeletedEdgesList(g: *mut Graph) -> *mut u64 {
+ if g.is_null() {
+ return std::ptr::null_mut();
+ }
(&*g).get_deleted_edges_list()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_GetDeletedEdgesList(g: *mut Graph) -> *mut u64 { | |
| (&*g).get_deleted_edges_list() | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_GetDeletedEdgesList(g: *mut Graph) -> *mut u64 { | |
| if g.is_null() { | |
| return std::ptr::null_mut(); | |
| } | |
| (&*g).get_deleted_edges_list() | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (7)
src/graph/graph.rs (7)
45-49: Consider adding a brief comment forMatrixPolicyThe
MatrixPolicyenum is introduced without any accompanying documentation. Consider adding a brief comment explaining its purpose and when each variant should be used. This will improve code readability and maintainability.
177-197: Document potential side effects ofapply_all_pendingThe
apply_all_pendingmethod temporarily changes the matrix policy, which could have side effects on concurrent operations. Consider adding a documentation comment to explain the purpose of this method and any potential side effects or thread-safety considerations.For example:
/// Applies all pending changes to the graph's matrices. /// /// This method temporarily changes the matrix policy to `FlushResize`, /// which may affect concurrent operations. Ensure proper synchronization /// when calling this method in a multi-threaded context. /// /// # Arguments /// /// * `force_flush` - If true, forces a flush of all pending changes. pub fn apply_all_pending(&mut self, force_flush: bool) { // ... existing implementation ... }
778-790: Fix typo in method namesyncronizeThe method name
syncronizecontains a typo. It should be spelledsynchronize. This typo should be corrected to improve code readability and maintain consistent naming conventions.fn synchronize( matrix_policy: MatrixPolicy, m: &mut DeltaMatrix, nrows: u64, ncols: u64, ) -> &mut DeltaMatrix { // ... existing implementation ... }Remember to update all calls to this method throughout the codebase.
1094-1105: Add comments explaining test initialization stepsThe
test_initfunction performs several initialization steps without explaining their purpose. Consider adding comments to clarify the reason for each step:fn test_init() { unsafe { // Reset memory allocator state for clean test environment Alloc_Reset(); // Initialize GraphBLAS in non-blocking mode GrB_init(GrB_Mode::GrB_NONBLOCKING); // Set GraphBLAS to use row-major format for better performance GxB_Global_Option_set(GxB_Option_Field::GxB_FORMAT, GxB_Format_Value::GxB_BY_ROW); // Set maximum pending changes for delta matrices Config_Option_set( ConfigOptionField::DELTA_MAX_PENDING_CHANGES, "10000\0".as_ptr() as *const c_char, null_mut(), ); }; }These comments will help other developers understand the purpose of each initialization step.
1107-1120: Enhance test coverage fortest_new_graphThe current test case for creating a new graph covers basic functionality, but it could be expanded to test more properties of the newly created graph. Consider adding more assertions:
#[test] fn test_new_graph() { test_init(); let mut g = Graph::new(16384, 16384); g.acquire_write_lock(); assert_eq!(g.adjacency_matrix.ncols(), g.required_matrix_dim()); assert_eq!(g.adjacency_matrix.nrows(), g.required_matrix_dim()); assert_eq!(g.adjacency_matrix.nvals(), 0); assert_eq!(g.node_count(), 0); // Add more assertions assert_eq!(g.edge_count(), 0); assert_eq!(g.label_type_count(), 0); assert_eq!(g.relation_type_count(), 0); assert!(!g.pending()); g.release_lock(); }These additional assertions will provide more comprehensive coverage of the initial graph state.
1122-1140: Enhance test coverage fortest_graph_constructionsThe current test case for graph construction covers basic node addition, but it could be expanded to test more properties after adding nodes. Consider adding more assertions:
#[test] fn test_graph_constructions() { test_init(); let node_count = 16384 / 2; let mut g = Graph::new(node_count, node_count); g.acquire_write_lock(); for _ in 0..node_count { let mut n = g.reserve_node(); g.create_node(&mut n, &[]); } assert_eq!(g.node_count(), node_count as u64); assert!(g.adjacency_matrix.ncols() >= node_count); assert!(g.adjacency_matrix.nrows() >= node_count); assert_eq!(g.adjacency_matrix.nvals(), 0); // Add more assertions assert_eq!(g.edge_count(), 0); assert_eq!(g.label_type_count(), 0); assert_eq!(g.relation_type_count(), 0); assert!(g.get_node(0).is_some()); assert!(g.get_node(node_count as u64 - 1).is_some()); assert!(g.get_node(node_count as u64).is_none()); g.release_lock(); }These additional assertions will provide more comprehensive coverage of the graph state after node addition.
1142-1183: Enhance test coverage fortest_remove_nodesThe current test case for removing nodes covers basic functionality, but it could be expanded to test more properties after node and edge removal. Consider adding more assertions:
#[test] fn test_remove_nodes() { test_init(); let mut g = Graph::new(32, 32); g.acquire_write_lock(); // ... existing setup code ... g.delete_edges(edges.as_mut_slice()); g.delete_nodes(&[n1]); g.release_lock(); assert_eq!(g.node_count(), 2); assert_eq!(g.edge_count(), 1); // Add more assertions assert!(g.get_node(n1.id).is_none()); assert!(g.get_node(n2.id).is_some()); assert!(g.get_node(n3.id).is_some()); let mut remaining_edges = Vec::new(); g.get_node_edges(&n2, GraphEdgeDir::Both, -1, &mut remaining_edges); assert_eq!(remaining_edges.len(), 1); assert_eq!(remaining_edges[0].src_id, n2.id); assert_eq!(remaining_edges[0].dest_id, n3.id); assert_eq!(g.get_relation_matrix(r, false).nvals(), 1); assert_eq!(g.get_adjacency_matrix(false).nvals(), 1); }These additional assertions will provide more comprehensive coverage of the graph state after node and edge removal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/graph/ffi.rs (1 hunks)
- src/graph/graph.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
src/graph/graph.rs (1)
1185-1200: Enhance test coverage fortest_get_nodeThe current test case for getting nodes covers basic functionality, but it could be expanded to test edge cases and invalid node retrieval. Consider adding more assertions:
src/graph/graph.rs
Outdated
| pub fn new( | ||
| node_cap: u64, | ||
| edge_cap: u64, | ||
| ) -> Self { | ||
| let nodes = unsafe { | ||
| DataBlock_New( | ||
| node_cap, | ||
| node_cap, | ||
| size_of::<AttributeSet>() as u32, | ||
| transmute_copy(&(AttributeSet_Free as *const c_void)), | ||
| ) | ||
| }; | ||
| let edges = unsafe { | ||
| DataBlock_New( | ||
| edge_cap, | ||
| edge_cap, | ||
| size_of::<AttributeSet>() as u32, | ||
| transmute_copy(&(AttributeSet_Free as *const c_void)), | ||
| ) | ||
| }; | ||
| Self { | ||
| reserved_node_count: 0, | ||
| nodes: nodes, | ||
| edges: edges, | ||
| adjacency_matrix: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, true), | ||
| labels: Vec::new(), | ||
| node_labels: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, false), | ||
| relations: Vec::new(), | ||
| zero_matrix: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, false), | ||
| rwlock: CRWLock::new(), | ||
| writelocked: false, | ||
| matrix_policy: MatrixPolicy::FlushResize, | ||
| stats: GraphStatistics { | ||
| node_count: Vec::new(), | ||
| edge_count: Vec::new(), | ||
| }, | ||
| partial: false, | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling in Graph::new method
The Graph::new method uses unsafe calls to DataBlock_New without checking if the returned pointers are null. This could lead to undefined behavior if the allocation fails.
Consider adding null checks and error handling for the DataBlock_New calls:
pub fn new(node_cap: u64, edge_cap: u64) -> Result<Self, &'static str> {
let nodes = unsafe {
// SAFETY: DataBlock_New is an FFI call that allocates memory.
// It's unsafe because it can return a null pointer if allocation fails.
let ptr = DataBlock_New(
node_cap,
node_cap,
size_of::<AttributeSet>() as u32,
transmute_copy(&(AttributeSet_Free as *const c_void)),
);
if ptr.is_null() {
return Err("Failed to allocate nodes DataBlock");
}
ptr
};
let edges = unsafe {
// SAFETY: DataBlock_New is an FFI call that allocates memory.
// It's unsafe because it can return a null pointer if allocation fails.
let ptr = DataBlock_New(
edge_cap,
edge_cap,
size_of::<AttributeSet>() as u32,
transmute_copy(&(AttributeSet_Free as *const c_void)),
);
if ptr.is_null() {
// Free the previously allocated nodes DataBlock
DataBlock_Free(nodes);
return Err("Failed to allocate edges DataBlock");
}
ptr
};
Ok(Self {
// ... struct fields ...
})
}This change will help prevent potential issues with failed allocations and improve the overall robustness of the code.
| pub fn create_edge( | ||
| &mut self, | ||
| src: NodeID, | ||
| dest: NodeID, | ||
| r: RelationID, | ||
| e: &mut Edge, | ||
| ) { | ||
| debug_assert!(self.get_node(src).is_some()); | ||
| debug_assert!(self.get_node(dest).is_some()); | ||
| debug_assert!(r >= 0 && r < self.relations.len() as i32); | ||
|
|
||
| e.src_id = src; | ||
| e.dest_id = dest; | ||
| e.relation_id = r; | ||
| e.attributes = unsafe { DataBlock_AllocateItem(self.edges, &mut e.id) } as _; | ||
|
|
||
| self.form_connection(src, dest, r, e); | ||
| } |
There was a problem hiding this comment.
Add error handling for DataBlock_AllocateItem call in create_edge
Similar to the create_node method, create_edge uses an unsafe call to DataBlock_AllocateItem without checking if the returned pointer is null. This could lead to undefined behavior if the allocation fails.
Consider adding a null check and error handling for the DataBlock_AllocateItem call:
pub fn create_edge(&mut self, src: NodeID, dest: NodeID, r: RelationID, e: &mut Edge) -> Result<(), &'static str> {
debug_assert!(self.get_node(src).is_some());
debug_assert!(self.get_node(dest).is_some());
debug_assert!(r >= 0 && r < self.relations.len() as i32);
e.src_id = src;
e.dest_id = dest;
e.relation_id = r;
e.attributes = unsafe {
let ptr = DataBlock_AllocateItem(self.edges, &mut e.id) as *mut AttributeSet;
if ptr.is_null() {
return Err("Failed to allocate edge");
}
ptr
};
self.form_connection(src, dest, r, e);
Ok(())
}This change ensures that any errors during edge allocation are properly handled and reported.
| pub fn scan_nodes(&self) -> *mut DataBlockIterator { | ||
| unsafe { DataBlock_Scan(self.nodes) } | ||
| } | ||
|
|
||
| pub fn scan_edges(&self) -> *mut DataBlockIterator { | ||
| unsafe { DataBlock_Scan(self.edges) } | ||
| } |
There was a problem hiding this comment.
Consider adding error handling for scan operations
The scan_nodes and scan_edges methods use unsafe FFI calls without any error handling. Consider adding error checking and returning a Result type:
pub fn scan_nodes(&self) -> Result<*mut DataBlockIterator, &'static str> {
let iterator = unsafe { DataBlock_Scan(self.nodes) };
if iterator.is_null() {
Err("Failed to create node iterator")
} else {
Ok(iterator)
}
}
pub fn scan_edges(&self) -> Result<*mut DataBlockIterator, &'static str> {
let iterator = unsafe { DataBlock_Scan(self.edges) };
if iterator.is_null() {
Err("Failed to create edge iterator")
} else {
Ok(iterator)
}
}This change will make the methods safer to use and allow callers to handle potential errors.
src/graph/graph.rs
Outdated
| pub fn get_node_labels( | ||
| &mut self, | ||
| n: &Node, | ||
| labels: &mut [i32], | ||
| ) -> u32 { | ||
| let m = self.get_node_label_matrix(); | ||
|
|
||
| let mut it = DeltaMatrixIter::new_range(m, n.id, n.id); | ||
| let mut i = 0; | ||
|
|
||
| while let Ok(Some((_, j))) = it.next_bool() { | ||
| labels[i as usize] = j as i32; | ||
| i += 1; | ||
| } | ||
|
|
||
| i | ||
| } |
There was a problem hiding this comment.
Improve safety and flexibility in get_node_labels
The get_node_labels method has a few potential issues:
- There's no bounds checking when writing to the
labelsarray, which could lead to buffer overflow. - The method returns a
u32, which might not be sufficient for large graphs.
Consider the following improvements:
pub fn get_node_labels(&mut self, n: &Node, labels: &mut Vec<i32>) -> usize {
let m = self.get_node_label_matrix();
let mut it = DeltaMatrixIter::new_range(m, n.id, n.id);
labels.clear(); // Ensure the vector is empty before adding new labels
while let Ok(Some((_, j))) = it.next_bool() {
labels.push(j as i32);
}
labels.len()
}This version:
- Uses a
Vec<i32>instead of a fixed-size array, preventing buffer overflow. - Returns a
usize, which is more appropriate for counting elements. - Clears the vector before adding new labels, ensuring a clean state.
| pub fn mark_edge_deleted( | ||
| &self, | ||
| id: u64, | ||
| ) { | ||
| unsafe { DataBlock_MarkAsDeletedOutOfOrder(self.edges, id) }; | ||
| } |
There was a problem hiding this comment.
Add error handling for DataBlock_MarkAsDeletedOutOfOrder
The mark_edge_deleted method uses an unsafe FFI call without any error handling. Consider adding error checking and returning a Result type:
pub fn mark_edge_deleted(&self, id: u64) -> Result<(), &'static str> {
let result = unsafe { DataBlock_MarkAsDeletedOutOfOrder(self.edges, id) };
if result != 0 { // Assuming 0 indicates success, adjust if needed
Err("Failed to mark edge as deleted")
} else {
Ok(())
}
}Apply similar changes to the mark_node_deleted method as well. This change will make the methods safer to use and allow callers to handle potential errors.
| it: *mut EdgeIterator, | ||
| e: *mut Edge, | ||
| ) -> bool { | ||
| (*it).next(e.as_mut().unwrap()) | ||
| } |
There was a problem hiding this comment.
Add null pointer checks in EdgeIterator_Next.
In EdgeIterator_Next, it and e are dereferenced without checking for null pointers. This can lead to undefined behavior if null pointers are passed.
Apply this diff to add null pointer checks:
unsafe extern "C" fn EdgeIterator_Next(
it: *mut EdgeIterator,
e: *mut Edge,
) -> bool {
+ if it.is_null() || e.is_null() {
+ return false; // Handle the error appropriately
+ }
(*it).next(e.as_mut().unwrap())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it: *mut EdgeIterator, | |
| e: *mut Edge, | |
| ) -> bool { | |
| (*it).next(e.as_mut().unwrap()) | |
| } | |
| unsafe extern "C" fn EdgeIterator_Next( | |
| it: *mut EdgeIterator, | |
| e: *mut Edge, | |
| ) -> bool { | |
| if it.is_null() || e.is_null() { | |
| return false; // Handle the error appropriately | |
| } | |
| (*it).next(e.as_mut().unwrap()) | |
| } |
src/graph/ffi.rs
Outdated
| g: *mut Graph, | ||
| it: *mut EdgeIterator, | ||
| srcID: NodeID, | ||
| destID: NodeID, | ||
| r: RelationID, | ||
| ) { | ||
| (&mut *it).init(g.as_mut().unwrap(), srcID, destID, r); | ||
| } |
There was a problem hiding this comment.
Add null pointer checks in Graph_EdgeIteratorInit.
In Graph_EdgeIteratorInit, g and it are dereferenced without checking for null pointers. This can lead to undefined behavior if null pointers are passed.
Apply this diff to add null pointer checks:
unsafe extern "C" fn Graph_EdgeIteratorInit(
g: *mut Graph,
it: *mut EdgeIterator,
srcID: NodeID,
destID: NodeID,
r: RelationID,
) {
+ if g.is_null() || it.is_null() {
+ return; // Handle the error appropriately
+ }
(&mut *it).init(g.as_mut().unwrap(), srcID, destID, r);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| g: *mut Graph, | |
| it: *mut EdgeIterator, | |
| srcID: NodeID, | |
| destID: NodeID, | |
| r: RelationID, | |
| ) { | |
| (&mut *it).init(g.as_mut().unwrap(), srcID, destID, r); | |
| } | |
| unsafe extern "C" fn Graph_EdgeIteratorInit( | |
| g: *mut Graph, | |
| it: *mut EdgeIterator, | |
| srcID: NodeID, | |
| destID: NodeID, | |
| r: RelationID, | |
| ) { | |
| if g.is_null() || it.is_null() { | |
| return; // Handle the error appropriately | |
| } | |
| (&mut *it).init(g.as_mut().unwrap(), srcID, destID, r); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 29
🧹 Outside diff range and nitpick comments (1)
src/graph/graph.rs (1)
42-57: Consider reviewing the usage of MatrixPolicy variants.The
MatrixPolicyenum is marked with#[allow(dead_code)], which suggests that some variants might not be used. It's worth reviewing if all variants are necessary or if some can be removed to improve code clarity.The
GraphEdgeDirenum looks well-defined for representing edge directions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- src/binding/cmutex.rs (1 hunks)
- src/graph/ffi.rs (1 hunks)
- src/graph/graph.rs (1 hunks)
- src/graph/matrix/delta_matrix.rs (7 hunks)
- src/graph/matrix/tensor.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/binding/cmutex.rs
🧰 Additional context used
🔇 Additional comments (9)
src/graph/ffi.rs (1)
511-523: Enhance safety in Graph_SetEdge function.The Graph_SetEdge function lacks null pointer checks and uses unwrap() on a potentially null pointer. Consider the following improvements:
- Add null pointer checks for the graph and edge pointers.
- Use a safer alternative to unwrap() when dealing with the potentially null edge pointer.
- Return
src/graph/graph.rs (2)
1-40: LGTM: Imports and constant definitions look good.The imports cover both standard library and custom modules, which seem appropriate for the graph implementation. The copyright notice is present, and the
INVALID_ENTITY_IDconstant is defined using a common practice.
1-1273: Overall assessment: Solid implementation with room for improvementThe
src/graph/graph.rsfile implements a complex graph data structure with various operations. The implementation is generally solid and includes unit tests, which is commendable. However, there are several areas where the code can be improved:
- Error handling: Many methods, especially those involving unsafe operations, could benefit from more robust error handling and reporting.
- Safety: Some unsafe operations lack proper documentation explaining their safety considerations. Adding such documentation would improve code maintainability and safety.
- Bounds checking: Some methods in
GraphStatisticsand elsewhere could benefit from additional bounds checking to prevent potential panics or undefined behavior.- Memory management: The
Dropimplementation forGraphcould be improved with better documentation and potentially more robust error handling during resource cleanup.Addressing these points would significantly enhance the overall quality, safety, and maintainability of the code.
src/graph/matrix/tensor.rs (1)
46-46: Confirm necessity ofunsafeusage inTensor::new.In line 46, the use of
unsafe { GrB_UINT64 }might not be necessary ifGrB_UINT64is already safe to use. Consider avoidingunsafeif possible or minimizing its scope.src/graph/matrix/delta_matrix.rs (5)
8-11: Proper use ofcmutex::CMutexfor mutex managementThe import of
cmutex::CMutexreplaces the custom mutex implementation, enhancing code reuse and maintainability by utilizing a centralized mutex module.
79-81: Consider returningOption<&DeltaMatrix>instead ofOption<&Box<DeltaMatrix>>As previously noted, returning
Option<&DeltaMatrix>simplifies the interface by avoiding unnecessary indirection and prevents exposing theBoxto the caller.
83-84: Consider returningOption<&mut DeltaMatrix>instead ofOption<&mut Box<DeltaMatrix>>This issue remains relevant from prior reviews. Refactoring to return
Option<&mut DeltaMatrix>would eliminate unnecessary indirection and improve the method's ergonomics.
103-106: Addition ofm_mutprovides necessary mutable accessThe new method
m_mutcorrectly offers mutable access toself.matrix, facilitating in-place modifications when required.
559-559: Variableashadows previous declaration intest_new_matrixAs previously mentioned, the variable
ais redeclared, which can lead to confusion and potential bugs. Consider renaming the secondato avoid shadowing.
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_ApplyAllPending( | ||
| g: *mut Graph, | ||
| force_flush: bool, | ||
| ) { | ||
| (&mut *g).apply_all_pending(force_flush); | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_GetMatrixPolicy(g: *mut Graph) -> MatrixPolicy { | ||
| (&*g).get_matrix_policy() | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_SetMatrixPolicy( | ||
| g: *mut Graph, | ||
| policy: MatrixPolicy, | ||
| ) -> MatrixPolicy { | ||
| (&mut *g).set_matrix_policy(policy) | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_Pending(g: *mut Graph) -> bool { | ||
| (&*g).pending() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance safety and error handling in graph operation functions.
The functions Graph_ApplyAllPending, Graph_GetMatrixPolicy, Graph_SetMatrixPolicy, and Graph_Pending lack null pointer checks and some lack error handling. Consider the following improvements:
- Add null pointer checks for the graph pointer in all functions.
- Implement error handling and return a boolean to indicate success/failure for Graph_ApplyAllPending.
Example implementation for Graph_ApplyAllPending:
#[no_mangle]
#[allow(non_snake_case)]
unsafe extern "C" fn Graph_ApplyAllPending(
g: *mut Graph,
force_flush: bool,
) -> bool {
if g.is_null() {
return false;
}
match (&mut *g).apply_all_pending(force_flush) {
Ok(_) => true,
Err(_) => false,
}
}Apply similar null pointer checks to the other functions in this group.
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_AddLabel(g: *mut Graph) -> LabelID { | ||
| (&mut *g).add_label() | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_LabelNode( | ||
| g: *mut Graph, | ||
| id: NodeID, | ||
| lbls: *mut LabelID, | ||
| lbl_count: u32, | ||
| ) { | ||
| (&mut *g).label_node(id, from_raw_parts(lbls, lbl_count as usize)); | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_RemoveNodeLabels( | ||
| g: *mut Graph, | ||
| id: NodeID, | ||
| lbls: *mut LabelID, | ||
| lbl_count: u32, | ||
| ) { | ||
| (&mut *g).remove_node_labels(id, from_raw_parts(lbls, lbl_count as usize)); | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_IsNodeLabeled( | ||
| g: *mut Graph, | ||
| id: NodeID, | ||
| l: LabelID, | ||
| ) -> bool { | ||
| (&mut *g).is_node_labeled(id, l) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve safety in label-related functions.
The label-related functions (Graph_AddLabel, Graph_LabelNode, Graph_RemoveNodeLabels, and Graph_IsNodeLabeled) have potential safety issues. Consider the following improvements:
- Add null pointer checks for the graph pointer in all functions.
- In Graph_LabelNode and Graph_RemoveNodeLabels, add checks for null lbls pointer when lbl_count is non-zero.
- Use a safer alternative to from_raw_parts.
Example improvement for Graph_LabelNode:
#[no_mangle]
#[allow(non_snake_case)]
unsafe extern "C" fn Graph_LabelNode(
g: *mut Graph,
id: NodeID,
lbls: *mut LabelID,
lbl_count: u32,
) -> bool {
if g.is_null() {
return false;
}
let labels = if lbl_count == 0 {
&[]
} else {
if lbls.is_null() {
return false;
}
std::slice::from_raw_parts(lbls, lbl_count as usize)
};
(&mut *g).label_node(id, labels);
true
}Apply similar changes to other functions in this group.
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_AddRelationType(g: *mut Graph) -> RelationID { | ||
| (&mut *g).add_relation_type() | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_AllocateNodes( | ||
| g: *mut Graph, | ||
| n: usize, | ||
| ) { | ||
| (&*g).allocate_nodes(n); | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_AllocateEdges( | ||
| g: *mut Graph, | ||
| n: usize, | ||
| ) { | ||
| (&*g).allocate_edges(n); | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_ResetReservedNode(g: *mut Graph) { | ||
| (&mut *g).reset_reserved_node(); | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_ReserveNode(g: *mut Graph) -> Node { | ||
| (&mut *g).reserve_node() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance safety and error handling in graph operation functions.
The functions Graph_AddRelationType, Graph_AllocateNodes, Graph_AllocateEdges, Graph_ResetReservedNode, and Graph_ReserveNode lack null pointer checks and some don't provide ways to handle potential errors. Consider the following improvements:
- Add null pointer checks for the graph pointer in all functions.
- Return boolean values to indicate success or failure, especially for operations that might fail (e.g., allocation).
- For Graph_ReserveNode, consider how to handle potential failures in node reservation.
Example improvements:
#[no_mangle]
#[allow(non_snake_case)]
unsafe extern "C" fn Graph_AllocateNodes(
g: *mut Graph,
n: usize,
) -> bool {
if g.is_null() {
return false;
}
match (&*g).allocate_nodes(n) {
Ok(_) => true,
Err(_) => false,
}
}
#[no_mangle]
#[allow(non_snake_case)]
unsafe extern "C" fn Graph_ReserveNode(g: *mut Graph) -> Option<Node> {
if g.is_null() {
return None;
}
(&mut *g).reserve_node().ok()
}Apply similar changes to other functions in this group.
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_DeleteNodes( | ||
| g: *mut Graph, | ||
| nodes: *mut Node, | ||
| count: u64, | ||
| ) { | ||
| (&mut *g).delete_nodes(from_raw_parts(nodes, count as usize)); | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_DeleteEdges( | ||
| g: *mut Graph, | ||
| edges: *mut Edge, | ||
| count: u64, | ||
| ) { | ||
| (&mut *g).delete_edges(from_raw_parts_mut(edges, count as usize)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance safety in node and edge deletion functions.
The functions Graph_DeleteNodes and Graph_DeleteEdges have potential safety issues. Consider the following improvements:
- Add null pointer checks for the graph and nodes/edges pointers.
- Use safer alternatives to from_raw_parts when dealing with potentially null pointers.
- Return a boolean to indicate success or failure of the operation.
Example improvements:
#[no_mangle]
#[allow(non_snake_case)]
unsafe extern "C" fn Graph_DeleteNodes(
g: *mut Graph,
nodes: *mut Node,
count: u64,
) -> bool {
if g.is_null() || nodes.is_null() {
return false;
}
let nodes_slice = std::slice::from_raw_parts(nodes, count as usize);
(&mut *g).delete_nodes(nodes_slice);
true
}
#[no_mangle]
#[allow(non_snake_case)]
unsafe extern "C" fn Graph_DeleteEdges(
g: *mut Graph,
edges: *mut Edge,
count: u64,
) -> bool {
if g.is_null() || edges.is_null() {
return false;
}
let edges_slice = std::slice::from_raw_parts_mut(edges, count as usize);
(&mut *g).delete_edges(edges_slice);
true
}These changes improve safety by checking for null pointers and provide a way for the caller to know if the operation was successful.
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_EntityIsDeleted(ge: *mut GraphEntity) -> bool { | ||
| if (&*ge).attributes.is_null() { | ||
| return false; | ||
| } | ||
|
|
||
| DataBlock_ItemIsDeleted((&*ge).attributes as _) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null pointer check in Graph_EntityIsDeleted.
The Graph_EntityIsDeleted function checks if the attributes pointer is null, but it doesn't check if the input pointer ge is null. Consider adding a null pointer check for ge:
#[no_mangle]
#[allow(non_snake_case)]
unsafe extern "C" fn Graph_EntityIsDeleted(ge: *mut GraphEntity) -> bool {
if ge.is_null() {
return false; // Or consider returning a special value to indicate error
}
if (&*ge).attributes.is_null() {
return false;
}
DataBlock_ItemIsDeleted((&*ge).attributes as _)
}This change will prevent potential crashes or undefined behavior when the function is called with a null pointer.
| unsafe { | ||
| GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut()); | ||
| GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0) | ||
| }; |
There was a problem hiding this comment.
Handle errors from iterator functions in TensorRangeIterator::next.
In the next method of TensorRangeIterator, the return values of GxB_Vector_Iterator_attach and GxB_Vector_Iterator_seek are ignored. Ensure that you check and handle these return values to prevent undefined behavior.
Apply this diff:
unsafe {
let info_attach = GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut());
+ if info_attach != GrB_Info::GrB_SUCCESS {
+ // Handle the error appropriately
+ return None;
+ }
let info_seek = GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0);
+ if info_seek != GrB_Info::GrB_SUCCESS {
+ // Handle the error appropriately
+ return None;
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| unsafe { | |
| GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut()); | |
| GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0) | |
| }; | |
| unsafe { | |
| let info_attach = GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut()); | |
| if info_attach != GrB_Info::GrB_SUCCESS { | |
| // Handle the error appropriately | |
| return None; | |
| } | |
| let info_seek = GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0); | |
| if info_seek != GrB_Info::GrB_SUCCESS { | |
| // Handle the error appropriately | |
| return None; | |
| } | |
| }; |
| GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut()); | ||
| let mut it = it.assume_init(); | ||
| GxB_Vector_Iterator_seek(&mut it, 0); | ||
| let index = GxB_Vector_Iterator_getIndex(&mut it); |
There was a problem hiding this comment.
Check for null pointer before using GxB_Vector_Iterator_getIndex.
In remove_elements, after attaching the iterator, you should verify that the iterator is valid before using it. This prevents potential null pointer dereferences.
Apply this diff to add a null pointer check:
let mut it = it.assume_init();
GxB_Vector_Iterator_seek(&mut it, 0);
+ if it.is_null() {
+ // Handle the error appropriately
+ return;
+ }
let index = GxB_Vector_Iterator_getIndex(&mut it);Committable suggestion was skipped due to low confidence.
| GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut()); | ||
| GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0) | ||
| }; | ||
| self.eit = Some(unsafe { it.assume_init() }); |
There was a problem hiding this comment.
Check return values of GraphBLAS iterator functions in TensorIterator::next.
In the next method of TensorIterator, the return values of GxB_Vector_Iterator_attach and GxB_Vector_Iterator_seek are not checked. It's important to handle potential errors from these functions.
Apply this diff to handle errors:
unsafe {
let info_attach = GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut());
+ if info_attach != GrB_Info::GrB_SUCCESS {
+ // Handle the error appropriately
+ // For example, set self.deleted = true and return None
+ self.deleted = true;
+ return None;
+ }
let info_seek = GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0);
+ if info_seek != GrB_Info::GrB_SUCCESS {
+ // Handle the error appropriately
+ self.deleted = true;
+ return None;
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut()); | |
| GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0) | |
| }; | |
| self.eit = Some(unsafe { it.assume_init() }); | |
| unsafe { | |
| let info_attach = GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut()); | |
| if info_attach != GrB_Info::GrB_SUCCESS { | |
| // Handle the error appropriately | |
| // For example, set self.deleted = true and return None | |
| self.deleted = true; | |
| return None; | |
| } | |
| let info_seek = GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0); | |
| if info_seek != GrB_Info::GrB_SUCCESS { | |
| // Handle the error appropriately | |
| self.deleted = true; | |
| return None; | |
| } | |
| }; | |
| self.eit = Some(unsafe { it.assume_init() }); |
|
|
||
| if to - from == 1 { | ||
| self.m | ||
| .set_element_u64(edges[from].as_ref().unwrap().id, src, dest); | ||
| } else { | ||
| let mut v = MaybeUninit::uninit(); | ||
| GrB_Vector_new(v.as_mut_ptr(), GrB_BOOL, GrB_INDEX_MAX); | ||
| let v = v.assume_init(); | ||
| self.m.set_element_u64(set_msb(v as u64), src, dest); | ||
| for j in from..to { | ||
| GrB_Vector_setElement_BOOL(v, true, edges[j].as_ref().unwrap().id); | ||
| } | ||
| GrB_Vector_wait(v, GrB_WaitMode::GrB_MATERIALIZE); | ||
| } |
There was a problem hiding this comment.
Free allocated GrB_Vector in set_elements to prevent memory leaks.
In the set_elements method, after using the GrB_Vector v, it should be freed to prevent memory leaks.
Apply this diff:
GrB_Vector_wait(v, GrB_WaitMode::GrB_MATERIALIZE);
+ GrB_Vector_free(&mut v);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if to - from == 1 { | |
| self.m | |
| .set_element_u64(edges[from].as_ref().unwrap().id, src, dest); | |
| } else { | |
| let mut v = MaybeUninit::uninit(); | |
| GrB_Vector_new(v.as_mut_ptr(), GrB_BOOL, GrB_INDEX_MAX); | |
| let v = v.assume_init(); | |
| self.m.set_element_u64(set_msb(v as u64), src, dest); | |
| for j in from..to { | |
| GrB_Vector_setElement_BOOL(v, true, edges[j].as_ref().unwrap().id); | |
| } | |
| GrB_Vector_wait(v, GrB_WaitMode::GrB_MATERIALIZE); | |
| } | |
| if to - from == 1 { | |
| self.m | |
| .set_element_u64(edges[from].as_ref().unwrap().id, src, dest); | |
| } else { | |
| let mut v = MaybeUninit::uninit(); | |
| GrB_Vector_new(v.as_mut_ptr(), GrB_BOOL, GrB_INDEX_MAX); | |
| let v = v.assume_init(); | |
| self.m.set_element_u64(set_msb(v as u64), src, dest); | |
| for j in from..to { | |
| GrB_Vector_setElement_BOOL(v, true, edges[j].as_ref().unwrap().id); | |
| } | |
| GrB_Vector_wait(v, GrB_WaitMode::GrB_MATERIALIZE); | |
| GrB_Vector_free(&mut v); | |
| } |
src/graph/matrix/delta_matrix.rs
Outdated
| @@ -522,7 +496,7 @@ impl DeltaMatrix { | |||
| return; | |||
| } | |||
|
|
|||
| self.mutex.as_mut().unwrap().lock(); | |||
| self.mutex.as_ref().unwrap().lock(); | |||
There was a problem hiding this comment.
Unwrapping self.mutex without checking may cause panics
In the synchronize method, calling self.mutex.as_ref().unwrap() assumes that self.mutex is always Some, which could lead to a panic if it's None. Ensure that self.mutex is always initialized or consider handling the Option more safely.
Apply this diff to handle potential None values:
- self.mutex.as_ref().unwrap().lock();
+ if let Some(mutex) = &self.mutex {
+ mutex.lock();
+ } else {
+ // Handle the None case appropriately
+ }Also applies to: 509-509
There was a problem hiding this comment.
Actionable comments posted: 22
🧹 Outside diff range and nitpick comments (12)
src/graph/matrix/tensor.rs (2)
24-34: Add documentation for bit manipulation utilities.
These utility functions handle critical edge ID operations but lack documentation explaining their purpose and usage.
Add rustdoc comments explaining:
+/// Returns true if the edge ID represents a single edge (MSB is not set).
pub fn single_edge(current_edge: EdgeID) -> bool {
current_edge as u64 & (1u64 << (u64::BITS as usize - 1)) == 0
}
+/// Sets the Most Significant Bit (MSB) of the edge ID to mark it as a multi-edge.
pub fn set_msb(meid: EdgeID) -> u64 {
meid as u64 | (1u64 << (u64::BITS as usize - 1))
}
+/// Clears the Most Significant Bit (MSB) of the edge ID to access the underlying value.
pub fn clear_msb(meid: EdgeID) -> u64 {
meid as u64 & !(1u64 << (u64::BITS as usize - 1))
}36-38: Add architectural documentation for the Tensor implementation.
The Tensor struct implements a complex graph data structure using bit manipulation and GraphBLAS, but lacks documentation explaining:
- The overall design and data representation
- How single vs. multiple edges are handled
- The relationship between Tensor and DeltaMatrix
- Memory management strategies
Add module-level documentation:
+/// # Tensor
+///
+/// A graph tensor implementation that efficiently handles both single and multiple edges
+/// between vertices using a combination of bit manipulation and GraphBLAS operations.
+///
+/// ## Design
+/// - Single edges are stored directly in the matrix
+/// - Multiple edges use the MSB to indicate a vector of edge IDs
+/// - The underlying DeltaMatrix provides efficient sparse matrix operations
+///
+/// ## Memory Management
+/// GraphBLAS vectors are allocated for multiple edges and must be freed when:
+/// - Removing edges
+/// - Converting from single to multiple edges
+/// - Finishing iteration over multiple edges
pub struct Tensor {
pub m: DeltaMatrix,
}src/graph/graph.rs (2)
1-38: Consider adding module-level documentation.
The file lacks module-level documentation explaining its purpose, key types, and usage patterns. Consider adding a doc comment at the module level to help users understand the graph implementation's capabilities and design choices.
Add documentation like this at the start of the file:
//! Graph implementation providing a concurrent, matrix-based representation of nodes and edges.
//!
//! This module implements a graph data structure using GraphBLAS matrices for efficient
//! storage and operations. It supports:
//! - Concurrent access through read-write locks
//! - Dynamic node and edge management
//! - Label and relationship type management
//! - Matrix-based graph operations
1058-1242: Enhance test coverage for error cases.
The tests primarily cover successful operations but should also verify error handling.
Consider adding tests for:
- Allocation failures
- Invalid node/edge IDs
- Concurrent access scenarios
- Resource exhaustion
- Error propagation
Example test:
#[test]
fn test_create_node_error_handling() {
test_init();
let mut g = Graph::new(1, 1).unwrap(); // Minimal capacity
g.acquire_write_lock();
// First node should succeed
let mut n1 = g.reserve_node();
assert!(g.create_node(&mut n1, &[]).is_ok());
// Second node should fail due to capacity
let mut n2 = g.reserve_node();
assert!(matches!(g.create_node(&mut n2, &[]), Err(_)));
g.release_lock();
}src/binding/crwlock.rs (1)
50-54: Check for errors in the Drop implementation.
In the Drop trait implementation, the call to pthread_rwlock_destroy does not handle potential errors. While failures are rare, it's good practice to check the return value to ensure the lock is destroyed properly.
Consider updating the drop method:
fn drop(&mut self) {
- unsafe { libc::pthread_rwlock_destroy(self.rwlock.get()) };
+ unsafe {
+ let res = libc::pthread_rwlock_destroy(self.rwlock.get());
+ debug_assert!(res == 0, "pthread_rwlock_destroy failed");
+ }
}src/graph/matrix/delta_matrix.rs (4)
22-25: Suggestion: Make DeltaMatrixBase Struct Private
The DeltaMatrixBase struct is currently declared as public, but it seems to be used only within this module. Making it private can encapsulate the implementation details and reduce the public API surface.
Apply this diff to make DeltaMatrixBase private:
-pub struct DeltaMatrixBase {
+struct DeltaMatrixBase {
pub matrix: SparseMatrix,
pub delta_plus: SparseMatrix,
pub delta_minus: SparseMatrix,
}35-37: Clarify the Purpose of the dirty Flag
The dirty flag is introduced to track pending changes, but it's not immediately clear how and when it's set or cleared. Adding documentation or comments would help other developers understand its role in the synchronization process.
Consider adding a comment like:
/// Indicates whether there are pending changes that require synchronization.
dirty: bool,Line range hint 279-305: Ensure Proper Resource Management in remove_elements Method
In the remove_elements method, GrB_Scalar_new and GrB_Scalar_free are used. Ensure that all allocated resources are properly freed, even in case of errors, to prevent memory leaks.
Review the code to confirm that GrB_Scalar_free is called in all execution paths.
Line range hint 573-583: Prevent Deadlocks by Ensuring Mutex is Always Released
In the synchronize method, self.mutex.lock() is called, but there is a return statement before self.mutex.unlock(), which can lead to a deadlock if the condition is met.
Apply this diff to use a scoped lock guard:
pub fn synchronize(
&mut self,
nrows: u64,
ncols: u64,
) {
if !(self.nrows() < nrows || self.ncols() < ncols || self.dirty) {
return;
}
self.mutex.lock();
// Ensure that unlock is called even if early returns happen
defer! {
self.mutex.unlock();
}
// Rest of the method...
}Alternatively, consider using RAII to automatically release the lock.
src/graph/matrix/ffi.rs (3)
Line range hint 17-22: Add null pointer check for output parameter a in Delta_Matrix_new
In the function Delta_Matrix_new, the output parameter a is dereferenced without checking if it is null. This can lead to undefined behavior if a is null.
Apply this diff to add a null pointer check:
unsafe extern "C" fn Delta_Matrix_new(
a: *mut *mut DeltaMatrix,
ty: GrB_Type,
nrows: GrB_Index,
ncols: GrB_Index,
transpose: bool,
) -> GrB_Info {
+ if a.is_null() {
+ return GrB_Info::GrB_NULL_POINTER;
+ }
*a = Box::into_raw(Box::new(DeltaMatrix::new(ty, nrows, ncols, transpose)));
GrB_Info::GrB_SUCCESS
}Line range hint 66-70: Add null pointer check for input parameter c in Delta_Matrix_setElement_BOOL
In Delta_Matrix_setElement_BOOL, the parameter c is dereferenced without checking if it is null.
Apply this diff:
unsafe extern "C" fn Delta_Matrix_setElement_BOOL(
c: *mut DeltaMatrix,
i: GrB_Index,
j: GrB_Index,
) -> GrB_Info {
+ if c.is_null() {
+ return GrB_Info::GrB_NULL_POINTER;
+ }
(*c).set_element_bool(i, j);
GrB_Info::GrB_SUCCESS
}Line range hint 75-83: Add null pointer checks in Delta_Matrix_extractElement_BOOL
Ensure that x and c are not null before dereferencing them.
Apply this diff:
unsafe extern "C" fn Delta_Matrix_extractElement_BOOL(
x: *mut bool,
c: *mut DeltaMatrix,
i: GrB_Index,
j: GrB_Index,
) -> GrB_Info {
+ if x.is_null() || c.is_null() {
+ return GrB_Info::GrB_NULL_POINTER;
+ }
if let Some(v) = (&*c).extract_element_bool(i, j) {
*x = v;
GrB_Info::GrB_SUCCESS📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- src/binding/crwlock.rs (1 hunks)
- src/graph/ffi.rs (1 hunks)
- src/graph/graph.rs (1 hunks)
- src/graph/matrix/delta_matrix.rs (33 hunks)
- src/graph/matrix/delta_matrix_iter.rs (4 hunks)
- src/graph/matrix/ffi.rs (5 hunks)
- src/graph/matrix/tensor.rs (1 hunks)
🔇 Additional comments (33)
src/graph/graph.rs (3)
122-160:
Improve error handling in Graph::new.
The constructor uses unsafe FFI calls without proper error handling, which could lead to undefined behavior if allocations fail.
Modify the constructor to handle allocation failures:
pub fn new(node_cap: u64, edge_cap: u64) -> Result<Self, &'static str> {
let nodes = unsafe {
let ptr = DataBlock_New(
node_cap,
node_cap,
size_of::<AttributeSet>() as u32,
transmute_copy(&(AttributeSet_Free as *const c_void)),
);
if ptr.is_null() {
return Err("Failed to allocate nodes DataBlock");
}
ptr
};
let edges = unsafe {
let ptr = DataBlock_New(
edge_cap,
edge_cap,
size_of::<AttributeSet>() as u32,
transmute_copy(&(AttributeSet_Free as *const c_void)),
);
if ptr.is_null() {
// Clean up previously allocated resources
DataBlock_Free(nodes);
return Err("Failed to allocate edges DataBlock");
}
ptr
};
Ok(Self { /* ... */ })
}Likely invalid or redundant comment.
59-103:
Add bounds checking to GraphStatistics methods.
The methods in GraphStatistics directly index into vectors without bounds checking, which could lead to panics if invalid indices are provided.
Consider adding bounds checking and returning Results:
impl GraphStatistics {
- fn increment_node_count(&mut self, l: LabelID, arg: u64) {
- self.node_count[l as usize] += arg;
+ fn increment_node_count(&mut self, l: LabelID, arg: u64) -> Result<(), &'static str> {
+ self.node_count.get_mut(l as usize)
+ .map(|count| *count += arg)
+ .ok_or("Invalid label ID")
}
- fn decrement_node_count(&mut self, l: LabelID, arg: u64) {
- self.node_count[l as usize] -= arg;
+ fn decrement_node_count(&mut self, l: LabelID, arg: u64) -> Result<(), &'static str> {
+ self.node_count.get_mut(l as usize)
+ .map(|count| *count = count.saturating_sub(arg))
+ .ok_or("Invalid label ID")
}
}Likely invalid or redundant comment.
894-929:
Improve safety in Drop implementation.
The Drop implementation contains unsafe code without proper error handling or documentation.
Add safety documentation and improve error handling:
impl Drop for Graph {
fn drop(&mut self) {
unsafe {
// SAFETY: DataBlock_FullScan and DataBlock_Scan are FFI calls that return an iterator.
// We need to ensure that we free all resources properly.
let it = if self.partial {
DataBlock_FullScan(self.nodes)
} else {
self.scan_nodes()
};
+ if it.is_null() {
+ // Log error but continue cleanup
+ eprintln!("Warning: Failed to create node iterator during cleanup");
+ }
// Free all node attribute sets
loop {
let set = DataBlockIterator_Next(it, null_mut()) as *mut AttributeSet;
if set.is_null() {
break;
}
AttributeSet_Free(set);
}
DataBlockIterator_Free(it);
// Similar error handling for edges cleanup
// ...
}
}
}Likely invalid or redundant comment.
src/binding/crwlock.rs (2)
17-26: Add error handling for pthread_rwlock_init in new().
The previous review comment about handling errors in new() is still applicable. Currently, debug_assert! is used, which only checks in debug builds. It's recommended to propagate initialization errors to the caller.
35-40: Add error handling for pthread_rwlock_wrlock in acquire_write().
The previous review comment regarding error handling in acquire_write() remains valid. Please consider propagating errors to the caller instead of using debug_assert!.
src/graph/matrix/delta_matrix_iter.rs (5)
20-25: Addition of 'transpose' parameter in new_range enhances flexibility
The inclusion of the transpose: bool parameter in the new_range constructor allows the DeltaMatrixIter to handle transposed matrices, increasing its versatility.
28-32: Correct utilization of transpose in iterator initialization
Using the transpose parameter when initializing m_it, dp_it, and dm_it ensures that the iterators correctly reflect the transposition state of the matrices.
42-42: Inclusion of transpose parameter in attach_range method
Adding the transpose: bool parameter to attach_range provides consistent control over the transposition state during reattachment, aligning with the changes made in new_range.
47-49: Proper initialization of iterators with transpose in attach_range
Updating m_it, dp_it, and dm_it with the transpose parameter ensures that the iterators are correctly configured when attach_range is called.
156-156: Test cases updated with transpose parameter
The test functions now pass false for the transpose parameter when creating a new DeltaMatrixIter, ensuring that the tests are aligned with the updated constructor.
Also applies to: 177-177
src/graph/ffi.rs (11)
23-25: Null pointer checks are missing in lock functions
The previous review comments about adding null pointer checks and error handling in Graph_AcquireReadLock, Graph_AcquireWriteLock, and Graph_ReleaseLock are still valid and applicable to these code segments.
Also applies to: 29-31, 35-37
50-52: Add null pointer checks in matrix policy functions
The earlier review comments regarding adding null pointer checks to Graph_GetMatrixPolicy and Graph_SetMatrixPolicy remain relevant for these functions.
Also applies to: 57-61
86-93: Ensure safe handling of label pointers in node label functions
The prior comments about potential null pointer dereferences in Graph_LabelNode and Graph_RemoveNodeLabels are still applicable. The issues concerning the handling of lbls when lbl_count is greater than zero need to be addressed.
Also applies to: 98-104
161-167: Null pointer checks needed for node and edge creation functions
Previous review comments about adding null pointer checks in Graph_CreateNode and Graph_CreateEdge are still valid. The use of as_mut().unwrap() without null checks can lead to panics if null pointers are passed.
Also applies to: 179-180
216-222: Add null pointer check in Graph_EntityIsDeleted
The earlier comment about the potential null pointer dereference in Graph_EntityIsDeleted remains applicable. Adding a null check for ge is necessary to enhance safety.
314-325: Null pointer checks required in Graph_GetNode and Graph_GetEdge
Previous comments regarding the need for null pointer checks in Graph_GetNode and Graph_GetEdge are still relevant. These functions should handle null pointers appropriately to prevent undefined behavior.
Also applies to: 330-335
352-356: Add null pointer checks in edge iterator functions
The past review comments about potential null pointer dereferences in EdgeIterator_Next and NodeEdgeIterator_Next are still valid. Ensuring that pointers are not null before dereferencing is critical.
Also applies to: 373-382
384-393: Ensure safety in node information retrieval functions
The previous concerns about null pointer handling in Graph_GetNodeDegree and Graph_GetNodeLabels remain applicable. Adding necessary null checks will enhance the safety of these functions.
Also applies to: 397-407
445-450: Potential issues in Graph_PartialFree and Graph_Free
Earlier comments about the risk of memory leaks or double-free errors in Graph_PartialFree and Graph_Free are still pertinent. Differentiating the two functions or consolidating them appropriately is recommended.
Also applies to: 452-456
479-483: Incorrect parameter type in Graph_MarkNodeDeleted
The previous review pointing out that the id parameter should likely be a NodeID instead of an EdgeID in Graph_MarkNodeDeleted remains valid.
536-547: Null pointer checks needed in tensor iterator functions
The past comments about adding null checks in TensorIterator_ScanRange, TensorIterator_next, and TensorIterator_is_attached are still applicable. Ensuring that pointers are validated before use is crucial for safety.
Also applies to: 552-571, 576-580
src/graph/matrix/delta_matrix.rs (11)
36-37: Potential Concurrency Issue with CMutex Usage
The mutex field is now a direct instance of CMutex instead of an Option. Ensure that the lock is properly acquired and released in all methods that modify shared data to prevent deadlocks or race conditions.
Review all methods that mutate shared state to confirm that they correctly use self.mutex.lock() and self.mutex.unlock(). Ensure that locks are not held longer than necessary.
216-234: Consistent Handling in set_element_u64 and set_element_bool
The logic in set_element_u64 differs from set_element_bool in how it updates matrix and delta_plus. Ensure that both methods handle additions and deletions consistently.
Review the logic in both methods to confirm they align, especially regarding updates to the transpose matrices.
247-252: Optimize Element Extraction Logic
The extract_element_bool method checks delta_plus, then delta_minus, then matrix. This can be inefficient if these matrices are large. Consider optimizing by using a more efficient data structure or caching mechanism.
[performance]
One option is to maintain a single combined matrix or to use a hash map for quick lookups.
846-847: Verify Correctness of test_fuzzy Results
In the test_fuzzy test, you compare a.matrix.matrix with m and a.transpose.unwrap().matrix with t. Ensure that all pending operations are flushed before comparison to get accurate results.
Call a.wait(true); before comparison to ensure all pending changes are applied.
Also, ensure that matrix_eq correctly compares the matrices, considering the potential presence of pending operations or differences in internal representations.
Line range hint 923-938: Clarify Logic in test_mxm
In the test_mxm test, c.mxm is called with b that has pending changes, whereas d.mxm is called after b.wait(true);. The test assumes that both results should be equal, but pending changes in b could lead to different outputs.
Ensure that the test accounts for pending changes. If mxm does not automatically flush pending operations, consider calling b.wait(true); before both mxm calls or clarify the expected behavior in comments.
998-1012: Ensure Correct Resizing in test_resize
In the test_resize test, after resizing the matrices, verify that all internal matrices (matrix, delta_plus, delta_minus) have been resized correctly, including in the transpose.
Add assertions to check the sizes of all internal matrices after resizing.
Example:
assert_eq!(a.matrix.matrix.nrows(), nrows);
assert_eq!(a.matrix.delta_plus.nrows(), nrows);
// Repeat for ncols and transpose matrices860-866: Ensure Consistency in test_export_no_changes
In the test_export_no_changes test, after exporting the matrix, confirm that n is identical to a.matrix.matrix, especially considering any pending operations.
If there are pending changes, ensure they are applied before comparison by calling a.wait(true);.
882-885: Validate Export Functionality with Pending Changes
In test_export_pending_changes, you export the matrix while there are pending changes. Verify that the exported matrix includes these changes as expected.
Consider adding assertions to compare n with the expected result, accounting for pending additions and deletions.
142-143:
Verify Correctness of nvals Calculation
The nvals method computes the number of values as:
self.matrix.matrix.nvals() + self.matrix.delta_plus.nvals() - self.matrix.delta_minus.nvals()This calculation assumes that there are no overlapping elements between delta_plus and delta_minus. If an element is both added and deleted, this might result in incorrect counts.
Create test cases where an element is added and then deleted (or vice versa) to ensure that nvals returns the correct value in all scenarios.
192-203:
Ensure Thread Safety in set_element_bool Method
The set_element_bool method modifies shared data without explicit synchronization. If DeltaMatrix is used in a concurrent context, this could lead to race conditions.
Confirm that access to shared mutable data is properly synchronized using the mutex. If necessary, wrap mutations in mutex locks:
self.mutex.lock();
// Mutation code
self.mutex.unlock();Alternatively, consider using Rust's concurrency primitives like std::sync::Mutex for safer thread handling.
661-671:
Correct the Assertions in test_simple_set
In the test_simple_set test, after calling a.wait(false);, the assertions check that a.matrix.matrix.nvals() is zero. However, depending on the behavior of wait(false), this may not be correct.
Review the purpose of wait(false) in this context and adjust the assertions accordingly. If wait(false) does not force synchronization, a.matrix.matrix.nvals() might still be zero.
Consider adjusting to:
assert_eq!(a.matrix.delta_plus.nvals(), 1);If synchronization is expected, use wait(true) instead.
src/graph/matrix/ffi.rs (1)
157-160: Avoid using read_unaligned() and write_unaligned() when alignment is guaranteed
The previous review comment regarding the use of read_unaligned() and write_unaligned() in Delta_Matrix_free is still applicable. Since pointers from Rust allocations are guaranteed to be aligned, it's safer and more idiomatic to use read() and write() instead.
Here is the suggested change:
unsafe extern "C" fn Delta_Matrix_free(c: *mut *mut DeltaMatrix) {
- drop(Box::from_raw(c.read_unaligned()));
- c.write_unaligned(null_mut());
+ drop(Box::from_raw(c.read()));
+ c.write(null_mut());
}| pub fn row_degree( | ||
| &self, | ||
| id: u64, | ||
| ) -> u64 { | ||
| let mut degree = 0; | ||
| let mut it = DeltaMatrixIter::new_range(&self.m, id, id, false); | ||
| while let Ok(Some((_, _, id))) = it.next_u64() { | ||
| if single_edge(id) { | ||
| degree += 1; | ||
| } else { | ||
| let v = clear_msb(id) as GrB_Vector; | ||
| let mut nvals = MaybeUninit::uninit(); | ||
| unsafe { | ||
| GrB_Vector_nvals(nvals.as_mut_ptr(), v); | ||
| degree += nvals.assume_init(); | ||
| } | ||
| } | ||
| } | ||
| degree | ||
| } |
There was a problem hiding this comment.
Add error handling for GraphBLAS operations in row_degree.
The method uses unsafe GraphBLAS operations without checking their return status.
Add error handling:
pub fn row_degree(&self, id: u64) -> u64 {
- let mut degree = 0;
+ let mut degree = 0u64;
let mut it = DeltaMatrixIter::new_range(&self.m, id, id, false);
while let Ok(Some((_, _, id))) = it.next_u64() {
if single_edge(id) {
degree += 1;
} else {
let v = clear_msb(id) as GrB_Vector;
let mut nvals = MaybeUninit::uninit();
unsafe {
- GrB_Vector_nvals(nvals.as_mut_ptr(), v);
- degree += nvals.assume_init();
+ match GrB_Vector_nvals(nvals.as_mut_ptr(), v) {
+ GrB_Info::GrB_SUCCESS => degree += nvals.assume_init(),
+ err => {
+ // Handle error appropriately
+ log::error!("Failed to get vector values: {:?}", err);
+ return 0;
+ }
+ }
}
}
}
degree
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn row_degree( | |
| &self, | |
| id: u64, | |
| ) -> u64 { | |
| let mut degree = 0; | |
| let mut it = DeltaMatrixIter::new_range(&self.m, id, id, false); | |
| while let Ok(Some((_, _, id))) = it.next_u64() { | |
| if single_edge(id) { | |
| degree += 1; | |
| } else { | |
| let v = clear_msb(id) as GrB_Vector; | |
| let mut nvals = MaybeUninit::uninit(); | |
| unsafe { | |
| GrB_Vector_nvals(nvals.as_mut_ptr(), v); | |
| degree += nvals.assume_init(); | |
| } | |
| } | |
| } | |
| degree | |
| } | |
| pub fn row_degree( | |
| &self, | |
| id: u64, | |
| ) -> u64 { | |
| let mut degree = 0u64; | |
| let mut it = DeltaMatrixIter::new_range(&self.m, id, id, false); | |
| while let Ok(Some((_, _, id))) = it.next_u64() { | |
| if single_edge(id) { | |
| degree += 1; | |
| } else { | |
| let v = clear_msb(id) as GrB_Vector; | |
| let mut nvals = MaybeUninit::uninit(); | |
| unsafe { | |
| match GrB_Vector_nvals(nvals.as_mut_ptr(), v) { | |
| GrB_Info::GrB_SUCCESS => degree += nvals.assume_init(), | |
| err => { | |
| // Handle error appropriately | |
| log::error!("Failed to get vector values: {:?}", err); | |
| return 0; | |
| } | |
| } | |
| } | |
| } | |
| } | |
| degree | |
| } |
| if let Some(current_edge) = self.m.extract_element_u64(src, dest) { | ||
| unsafe { | ||
| if single_edge(current_edge) { | ||
| let mut v = MaybeUninit::uninit(); | ||
| GrB_Vector_new(v.as_mut_ptr(), GrB_BOOL, GrB_INDEX_MAX); | ||
| let v = v.assume_init(); | ||
| self.m.set_element_u64(set_msb(v as u64), src, dest); | ||
| GrB_Vector_setElement_BOOL(v, true, current_edge); | ||
| GrB_Vector_setElement_BOOL(v, true, id); | ||
| GrB_Vector_wait(v, GrB_WaitMode::GrB_MATERIALIZE); | ||
| } else { | ||
| let v = clear_msb(current_edge) as GrB_Vector; | ||
| GrB_Vector_setElement_BOOL(v, true, id); | ||
| GrB_Vector_wait(v, GrB_WaitMode::GrB_MATERIALIZE); | ||
| } | ||
| } | ||
| } else { | ||
| self.m.set_element_u64(id, src, dest); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix memory leak in set_element.
The GrB_Vector created in the if single_edge branch is never freed.
Add vector cleanup:
GrB_Vector_setElement_BOOL(v, true, current_edge);
GrB_Vector_setElement_BOOL(v, true, id);
GrB_Vector_wait(v, GrB_WaitMode::GrB_MATERIALIZE);
+ GrB_Vector_free(&mut v);
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(current_edge) = self.m.extract_element_u64(src, dest) { | |
| unsafe { | |
| if single_edge(current_edge) { | |
| let mut v = MaybeUninit::uninit(); | |
| GrB_Vector_new(v.as_mut_ptr(), GrB_BOOL, GrB_INDEX_MAX); | |
| let v = v.assume_init(); | |
| self.m.set_element_u64(set_msb(v as u64), src, dest); | |
| GrB_Vector_setElement_BOOL(v, true, current_edge); | |
| GrB_Vector_setElement_BOOL(v, true, id); | |
| GrB_Vector_wait(v, GrB_WaitMode::GrB_MATERIALIZE); | |
| } else { | |
| let v = clear_msb(current_edge) as GrB_Vector; | |
| GrB_Vector_setElement_BOOL(v, true, id); | |
| GrB_Vector_wait(v, GrB_WaitMode::GrB_MATERIALIZE); | |
| } | |
| } | |
| } else { | |
| self.m.set_element_u64(id, src, dest); | |
| } | |
| } | |
| if let Some(current_edge) = self.m.extract_element_u64(src, dest) { | |
| unsafe { | |
| if single_edge(current_edge) { | |
| let mut v = MaybeUninit::uninit(); | |
| GrB_Vector_new(v.as_mut_ptr(), GrB_BOOL, GrB_INDEX_MAX); | |
| let v = v.assume_init(); | |
| self.m.set_element_u64(set_msb(v as u64), src, dest); | |
| GrB_Vector_setElement_BOOL(v, true, current_edge); | |
| GrB_Vector_setElement_BOOL(v, true, id); | |
| GrB_Vector_wait(v, GrB_WaitMode::GrB_MATERIALIZE); | |
| GrB_Vector_free(&mut v); | |
| } else { | |
| let v = clear_msb(current_edge) as GrB_Vector; | |
| GrB_Vector_setElement_BOOL(v, true, id); | |
| GrB_Vector_wait(v, GrB_WaitMode::GrB_MATERIALIZE); | |
| } | |
| } | |
| } else { | |
| self.m.set_element_u64(id, src, dest); | |
| } | |
| } |
| impl Iterator for TensorIterator { | ||
| type Item = (NodeID, NodeID, EdgeID); | ||
|
|
||
| fn next(&mut self) -> Option<Self::Item> { | ||
| if self.deleted { | ||
| return None; | ||
| } | ||
| if single_edge(self.edge_id) { | ||
| self.deleted = true; | ||
| Some((self.src_id, self.dest_id, self.edge_id)) | ||
| } else { | ||
| if self.eit.is_none() { | ||
| let v = clear_msb(self.edge_id) as GrB_Vector; | ||
| let mut it = MaybeUninit::uninit(); | ||
| unsafe { | ||
| GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut()); | ||
| GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0) | ||
| }; | ||
| self.eit = Some(unsafe { it.assume_init() }); | ||
| } | ||
| unsafe { | ||
| let edge_id = GxB_Vector_Iterator_getIndex(self.eit.as_mut().unwrap()); | ||
| let info = GxB_Vector_Iterator_next(self.eit.as_mut().unwrap()); | ||
| if info == GrB_Info::GxB_EXHAUSTED { | ||
| self.eit = None; | ||
| self.deleted = true; | ||
| } | ||
| return Some((self.src_id, self.dest_id, edge_id)); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve safety in TensorIterator::next.
The iterator implementation has several safety concerns:
- No null pointer checks after iterator attachment
- Unchecked GraphBLAS operations
- Potential use-after-free when accessing the vector
Add safety checks:
fn next(&mut self) -> Option<Self::Item> {
if self.deleted {
return None;
}
if single_edge(self.edge_id) {
self.deleted = true;
Some((self.src_id, self.dest_id, self.edge_id))
} else {
if self.eit.is_none() {
let v = clear_msb(self.edge_id) as GrB_Vector;
let mut it = MaybeUninit::uninit();
unsafe {
- GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut());
- GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0)
+ if GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut()) != GrB_Info::GrB_SUCCESS {
+ self.deleted = true;
+ return None;
+ }
+ if GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0) != GrB_Info::GrB_SUCCESS {
+ self.deleted = true;
+ return None;
+ }
};
self.eit = Some(unsafe { it.assume_init() });
}
unsafe {
+ let Some(it) = self.eit.as_mut() else {
+ return None;
+ };
- let edge_id = GxB_Vector_Iterator_getIndex(self.eit.as_mut().unwrap());
- let info = GxB_Vector_Iterator_next(self.eit.as_mut().unwrap());
+ let edge_id = GxB_Vector_Iterator_getIndex(it);
+ let info = GxB_Vector_Iterator_next(it);
if info == GrB_Info::GxB_EXHAUSTED {
self.eit = None;
self.deleted = true;
}
return Some((self.src_id, self.dest_id, edge_id));
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl Iterator for TensorIterator { | |
| type Item = (NodeID, NodeID, EdgeID); | |
| fn next(&mut self) -> Option<Self::Item> { | |
| if self.deleted { | |
| return None; | |
| } | |
| if single_edge(self.edge_id) { | |
| self.deleted = true; | |
| Some((self.src_id, self.dest_id, self.edge_id)) | |
| } else { | |
| if self.eit.is_none() { | |
| let v = clear_msb(self.edge_id) as GrB_Vector; | |
| let mut it = MaybeUninit::uninit(); | |
| unsafe { | |
| GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut()); | |
| GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0) | |
| }; | |
| self.eit = Some(unsafe { it.assume_init() }); | |
| } | |
| unsafe { | |
| let edge_id = GxB_Vector_Iterator_getIndex(self.eit.as_mut().unwrap()); | |
| let info = GxB_Vector_Iterator_next(self.eit.as_mut().unwrap()); | |
| if info == GrB_Info::GxB_EXHAUSTED { | |
| self.eit = None; | |
| self.deleted = true; | |
| } | |
| return Some((self.src_id, self.dest_id, edge_id)); | |
| } | |
| } | |
| } | |
| } | |
| impl Iterator for TensorIterator { | |
| type Item = (NodeID, NodeID, EdgeID); | |
| fn next(&mut self) -> Option<Self::Item> { | |
| if self.deleted { | |
| return None; | |
| } | |
| if single_edge(self.edge_id) { | |
| self.deleted = true; | |
| Some((self.src_id, self.dest_id, self.edge_id)) | |
| } else { | |
| if self.eit.is_none() { | |
| let v = clear_msb(self.edge_id) as GrB_Vector; | |
| let mut it = MaybeUninit::uninit(); | |
| unsafe { | |
| if GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut()) != GrB_Info::GrB_SUCCESS { | |
| self.deleted = true; | |
| return None; | |
| } | |
| if GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0) != GrB_Info::GrB_SUCCESS { | |
| self.deleted = true; | |
| return None; | |
| } | |
| }; | |
| self.eit = Some(unsafe { it.assume_init() }); | |
| } | |
| unsafe { | |
| let Some(it) = self.eit.as_mut() else { | |
| return None; | |
| }; | |
| let edge_id = GxB_Vector_Iterator_getIndex(it); | |
| let info = GxB_Vector_Iterator_next(it); | |
| if info == GrB_Info::GxB_EXHAUSTED { | |
| self.eit = None; | |
| self.deleted = true; | |
| } | |
| return Some((self.src_id, self.dest_id, edge_id)); | |
| } | |
| } | |
| } | |
| } |
src/binding/crwlock.rs
Outdated
| pub fn release(&self) { | ||
| unsafe { | ||
| let res = libc::pthread_rwlock_unlock(self.rwlock.get()); | ||
| debug_assert!(res == 0, "pthread_rwlock_unlock failed"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle errors in release().
Similar to the other methods, release() uses debug_assert! to check the result of pthread_rwlock_unlock, which won't catch errors in release builds. To ensure robustness, modify the method to return a Result<(), std::io::Error> and handle any unlocking errors appropriately.
Here's how you can adjust the method:
- pub fn release(&self) {
+ pub fn release(&self) -> Result<(), std::io::Error> {
unsafe {
let res = libc::pthread_rwlock_unlock(self.rwlock.get());
- debug_assert!(res == 0, "pthread_rwlock_unlock failed");
+ if res != 0 {
+ Err(std::io::Error::from_raw_os_error(res))
+ } else {
+ Ok(())
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn release(&self) { | |
| unsafe { | |
| let res = libc::pthread_rwlock_unlock(self.rwlock.get()); | |
| debug_assert!(res == 0, "pthread_rwlock_unlock failed"); | |
| } | |
| } | |
| pub fn release(&self) -> Result<(), std::io::Error> { | |
| unsafe { | |
| let res = libc::pthread_rwlock_unlock(self.rwlock.get()); | |
| if res != 0 { | |
| Err(std::io::Error::from_raw_os_error(res)) | |
| } else { | |
| Ok(()) | |
| } | |
| } | |
| } |
src/binding/crwlock.rs
Outdated
| pub fn acquire_read(&self) { | ||
| unsafe { | ||
| let res = libc::pthread_rwlock_rdlock(self.rwlock.get()); | ||
| debug_assert!(res == 0, "pthread_rwlock_rdlock failed"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Propagate errors in acquire_read().
Currently, acquire_read() uses debug_assert! to check the result of pthread_rwlock_rdlock, which only operates in debug builds. In release builds, failures in acquiring the read lock would go unnoticed. Consider modifying the method to return a Result<(), std::io::Error> and propagate errors to the caller.
Apply the following changes to handle errors properly:
- pub fn acquire_read(&self) {
+ pub fn acquire_read(&self) -> Result<(), std::io::Error> {
unsafe {
let res = libc::pthread_rwlock_rdlock(self.rwlock.get());
- debug_assert!(res == 0, "pthread_rwlock_rdlock failed");
+ if res != 0 {
+ Err(std::io::Error::from_raw_os_error(res))
+ } else {
+ Ok(())
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn acquire_read(&self) { | |
| unsafe { | |
| let res = libc::pthread_rwlock_rdlock(self.rwlock.get()); | |
| debug_assert!(res == 0, "pthread_rwlock_rdlock failed"); | |
| } | |
| } | |
| pub fn acquire_read(&self) -> Result<(), std::io::Error> { | |
| unsafe { | |
| let res = libc::pthread_rwlock_rdlock(self.rwlock.get()); | |
| if res != 0 { | |
| Err(std::io::Error::from_raw_os_error(res)) | |
| } else { | |
| Ok(()) | |
| } | |
| } | |
| } |
| c: *mut DeltaMatrix, | ||
| ) -> GrB_Info { | ||
| *nvals = (&*c).nvals(); | ||
| GrB_Info::GrB_SUCCESS | ||
| } |
There was a problem hiding this comment.
Add null pointer checks in Delta_Matrix_nvals
In Delta_Matrix_nvals, check for null pointers before dereferencing nvals and c.
Apply this diff:
unsafe extern "C" fn Delta_Matrix_nvals(
nvals: *mut GrB_Index,
c: *mut DeltaMatrix,
) -> GrB_Info {
+ if nvals.is_null() || c.is_null() {
+ return GrB_Info::GrB_NULL_POINTER;
+ }
*nvals = (&*c).nvals();
GrB_Info::GrB_SUCCESS
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| c: *mut DeltaMatrix, | |
| ) -> GrB_Info { | |
| *nvals = (&*c).nvals(); | |
| GrB_Info::GrB_SUCCESS | |
| } | |
| unsafe extern "C" fn Delta_Matrix_nvals( | |
| nvals: *mut GrB_Index, | |
| c: *mut DeltaMatrix, | |
| ) -> GrB_Info { | |
| if nvals.is_null() || c.is_null() { | |
| return GrB_Info::GrB_NULL_POINTER; | |
| } | |
| *nvals = (&*c).nvals(); | |
| GrB_Info::GrB_SUCCESS | |
| } |
| c: *mut DeltaMatrix, | ||
| ) -> GrB_Info { | ||
| *nrows = (&*c).nrows(); | ||
| GrB_Info::GrB_SUCCESS | ||
| } |
There was a problem hiding this comment.
Add null pointer checks in Delta_Matrix_nrows
In Delta_Matrix_nrows, the input parameters nrows and c are dereferenced without null checks. This can lead to undefined behavior if either is null.
Apply this diff:
unsafe extern "C" fn Delta_Matrix_nrows(
nrows: *mut GrB_Index,
c: *mut DeltaMatrix,
) -> GrB_Info {
+ if nrows.is_null() || c.is_null() {
+ return GrB_Info::GrB_NULL_POINTER;
+ }
*nrows = (&*c).nrows();
GrB_Info::GrB_SUCCESS
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| c: *mut DeltaMatrix, | |
| ) -> GrB_Info { | |
| *nrows = (&*c).nrows(); | |
| GrB_Info::GrB_SUCCESS | |
| } | |
| unsafe extern "C" fn Delta_Matrix_nrows( | |
| nrows: *mut GrB_Index, | |
| c: *mut DeltaMatrix, | |
| ) -> GrB_Info { | |
| if nrows.is_null() || c.is_null() { | |
| return GrB_Info::GrB_NULL_POINTER; | |
| } | |
| *nrows = (&*c).nrows(); | |
| GrB_Info::GrB_SUCCESS | |
| } |
| c: *mut DeltaMatrix, | ||
| semiring: GrB_Semiring, | ||
| a: _Matrix, | ||
| b: _Matrix, | ||
| a: *mut DeltaMatrix, | ||
| b: *mut DeltaMatrix, | ||
| desc: GrB_Descriptor, | ||
| ) -> GrB_Info { | ||
| (*c).element_wise_add(semiring, &*a, &*b); | ||
| (*c).element_wise_add(semiring, &*a, &*b, desc); | ||
| GrB_Info::GrB_SUCCESS | ||
| } | ||
|
|
There was a problem hiding this comment.
Add null pointer checks in Delta_eWiseAdd
In Delta_eWiseAdd, check that c, a, and b are not null before use.
Apply this diff:
unsafe extern "C" fn Delta_eWiseAdd(
c: *mut DeltaMatrix,
semiring: GrB_Semiring,
a: *mut DeltaMatrix,
b: *mut DeltaMatrix,
desc: GrB_Descriptor,
) -> GrB_Info {
+ if c.is_null() || a.is_null() || b.is_null() {
+ return GrB_Info::GrB_NULL_POINTER;
+ }
(*c).element_wise_add(semiring, &*a, &*b, desc);
GrB_Info::GrB_SUCCESS
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| c: *mut DeltaMatrix, | |
| semiring: GrB_Semiring, | |
| a: _Matrix, | |
| b: _Matrix, | |
| a: *mut DeltaMatrix, | |
| b: *mut DeltaMatrix, | |
| desc: GrB_Descriptor, | |
| ) -> GrB_Info { | |
| (*c).element_wise_add(semiring, &*a, &*b); | |
| (*c).element_wise_add(semiring, &*a, &*b, desc); | |
| GrB_Info::GrB_SUCCESS | |
| } | |
| unsafe extern "C" fn Delta_eWiseAdd( | |
| c: *mut DeltaMatrix, | |
| semiring: GrB_Semiring, | |
| a: *mut DeltaMatrix, | |
| b: *mut DeltaMatrix, | |
| desc: GrB_Descriptor, | |
| ) -> GrB_Info { | |
| if c.is_null() || a.is_null() || b.is_null() { | |
| return GrB_Info::GrB_NULL_POINTER; | |
| } | |
| (*c).element_wise_add(semiring, &*a, &*b, desc); | |
| GrB_Info::GrB_SUCCESS | |
| } |
| unsafe extern "C" fn Delta_Matrix_M(c: *mut DeltaMatrix) -> GrB_Matrix { | ||
| (&*c).m(false).grb_matrix_ref() | ||
| } |
There was a problem hiding this comment.
Add null pointer check for input parameter c in Delta_Matrix_M
In the function Delta_Matrix_M, the input parameter c is dereferenced without checking if it is null, which could lead to undefined behavior.
Apply this diff to add a null pointer check:
unsafe extern "C" fn Delta_Matrix_M(c: *mut DeltaMatrix) -> GrB_Matrix {
+ if c.is_null() {
+ return null_mut();
+ }
(&*c).m(false).grb_matrix_ref()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| unsafe extern "C" fn Delta_Matrix_M(c: *mut DeltaMatrix) -> GrB_Matrix { | |
| (&*c).m(false).grb_matrix_ref() | |
| } | |
| unsafe extern "C" fn Delta_Matrix_M(c: *mut DeltaMatrix) -> GrB_Matrix { | |
| if c.is_null() { | |
| return null_mut(); | |
| } | |
| (&*c).m(false).grb_matrix_ref() | |
| } |
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Delta_mxm( | ||
| c: _Matrix, | ||
| c: *mut DeltaMatrix, | ||
| semiring: GrB_Semiring, | ||
| a: _Matrix, | ||
| b: _Matrix, | ||
| a: *mut DeltaMatrix, | ||
| b: *mut DeltaMatrix, | ||
| desc: GrB_Descriptor, | ||
| ) -> GrB_Info { | ||
| (*c).mxm(semiring, &*a, &*b); | ||
| (*c).mxm(semiring, &*a, &*b, desc); | ||
| GrB_Info::GrB_SUCCESS |
There was a problem hiding this comment.
Add null pointer checks in Delta_mxm
In Delta_mxm, input parameters c, a, and b are dereferenced without null checks.
Apply this diff:
unsafe extern "C" fn Delta_mxm(
c: *mut DeltaMatrix,
semiring: GrB_Semiring,
a: *mut DeltaMatrix,
b: *mut DeltaMatrix,
desc: GrB_Descriptor,
) -> GrB_Info {
+ if c.is_null() || a.is_null() || b.is_null() {
+ return GrB_Info::GrB_NULL_POINTER;
+ }
(*c).mxm(semiring, &*a, &*b, desc);
GrB_Info::GrB_SUCCESS
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Delta_mxm( | |
| c: _Matrix, | |
| c: *mut DeltaMatrix, | |
| semiring: GrB_Semiring, | |
| a: _Matrix, | |
| b: _Matrix, | |
| a: *mut DeltaMatrix, | |
| b: *mut DeltaMatrix, | |
| desc: GrB_Descriptor, | |
| ) -> GrB_Info { | |
| (*c).mxm(semiring, &*a, &*b); | |
| (*c).mxm(semiring, &*a, &*b, desc); | |
| GrB_Info::GrB_SUCCESS | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Delta_mxm( | |
| c: *mut DeltaMatrix, | |
| semiring: GrB_Semiring, | |
| a: *mut DeltaMatrix, | |
| b: *mut DeltaMatrix, | |
| desc: GrB_Descriptor, | |
| ) -> GrB_Info { | |
| if c.is_null() || a.is_null() || b.is_null() { | |
| return GrB_Info::GrB_NULL_POINTER; | |
| } | |
| (*c).mxm(semiring, &*a, &*b, desc); | |
| GrB_Info::GrB_SUCCESS |
This give some performance advantages as well as ergonomic advantage as rust code can use the underline parking_lot object with more safe and idiomatic API.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (6)
src/graph/matrix/tensor.rs (3)
24-34: Add documentation for bit manipulation utilities.These utility functions are crucial for edge ID manipulation but lack documentation explaining their purpose and usage. Consider adding doc comments to explain:
- The significance of the MSB in edge identification
- When and why these functions should be used
- The relationship between single edges and multi-edges
40-48: Consider using const for GrB_UINT64.The unsafe block in
new()could be replaced with a const declaration at module level:+const GRB_UINT64_TYPE: GrB_Type = unsafe { GrB_UINT64 }; impl Tensor { pub fn new(nrows: u64, ncols: u64) -> Self { Self { - m: DeltaMatrix::new(unsafe { GrB_UINT64 }, nrows, ncols, true), + m: DeltaMatrix::new(GRB_UINT64_TYPE, nrows, ncols, true), } } }
312-353: Consider refactoring degree calculations to reduce code duplication.The
row_degreeandcol_degreemethods share similar logic. Consider extracting the common functionality into a private helper method:+ fn calculate_degree(&self, id: u64, transposed: bool) -> u64 { + let mut degree = 0; + let mut it = DeltaMatrixIter::new_range(&self.m, id, id, transposed); + + while let Ok(Some((src, dest))) = if transposed { + it.next_bool().map(|r| r.map(|(d, s)| (s, d))) + } else { + it.next_u64().map(|r| r.map(|(s, d, _)| (s, d))) + } { + let id = self.m.extract_element_u64(src, dest).unwrap(); + if single_edge(id) { + degree += 1; + } else { + let v = clear_msb(id) as GrB_Vector; + let mut nvals = MaybeUninit::uninit(); + unsafe { + GrB_Vector_nvals(nvals.as_mut_ptr(), v); + degree += nvals.assume_init(); + } + } + } + degree + } + + pub fn row_degree(&self, id: u64) -> u64 { + self.calculate_degree(id, false) + } + + pub fn col_degree(&self, id: u64) -> u64 { + self.calculate_degree(id, true) + }src/graph/graph.rs (2)
12-24: Add documentation for unsafe FFI importsThe code imports several unsafe FFI functions without documenting their safety requirements and invariants. Consider adding module-level documentation explaining the safety requirements for these FFI functions.
1039-1223: Improve test coverage for error conditionsThe current tests cover basic functionality but lack tests for:
- Error conditions in matrix operations
- Edge cases with invalid inputs
- Concurrent access scenarios
- Memory allocation failures
Consider adding tests for these scenarios to ensure robust error handling.
src/binding/graph.rs (1)
62-68: Consider refactoring to reduce code duplication inset_attributesmethodsBoth
NodeandEdgestructs have identicalset_attributesmethods. Since you have introduced theGraphEntitystruct, consider refactoring to haveNodeandEdgeleverageGraphEntityto share this common functionality. This adheres to the DRY (Don't Repeat Yourself) principle and improves maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/binding/graph.rs(4 hunks)src/graph/ffi.rs(1 hunks)src/graph/graph.rs(1 hunks)src/graph/matrix/tensor.rs(1 hunks)
🔇 Additional comments (2)
src/binding/graph.rs (2)
10-12: Verify impact of changing ID types to unsigned integers
The types NodeID, EdgeID, and EntityID have been changed from i64 to u64. Changing from signed to unsigned integers may introduce issues if negative values are used or if there is existing code that relies on signed integer behavior. Please ensure that all usages of these types in the codebase are compatible with the new unsigned types to prevent potential integer overflows or logic errors.
218-218: Ensure proper memory management for returned pointer
The function DataBlock_DeletedItems returns a raw pointer to u64. It's important to manage the lifetime and ownership of this pointer correctly to prevent memory leaks or undefined behavior. Please verify that the calling code appropriately handles the allocation and deallocation of this memory.
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_ApplyAllPending( | ||
| g: *mut Graph, | ||
| force_flush: bool, | ||
| ) { | ||
| (&mut *g).apply_all_pending(force_flush); | ||
| } |
There was a problem hiding this comment.
Add error handling for pending operations.
The Graph_ApplyAllPending function doesn't handle potential errors from the underlying operation, which could lead to silent failures.
Consider returning a boolean to indicate success/failure:
unsafe extern "C" fn Graph_ApplyAllPending(
g: *mut Graph,
force_flush: bool,
-) {
+) -> bool {
+ if g.is_null() {
+ return false;
+ }
- (&mut *g).apply_all_pending(force_flush);
+ match (&mut *g).apply_all_pending(force_flush) {
+ Ok(_) => true,
+ Err(_) => false,
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_AllocateNodes( | ||
| g: *mut Graph, | ||
| n: usize, | ||
| ) { | ||
| (&*g).allocate_nodes(n); | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_AllocateEdges( | ||
| g: *mut Graph, | ||
| n: usize, | ||
| ) { | ||
| (&*g).allocate_edges(n); | ||
| } |
There was a problem hiding this comment.
Add allocation failure handling.
The allocation functions don't handle potential allocation failures, which could lead to crashes.
Add error handling:
unsafe extern "C" fn Graph_AllocateNodes(
g: *mut Graph,
n: usize,
-) {
+) -> bool {
+ if g.is_null() {
+ return false;
+ }
- (&*g).allocate_nodes(n);
+ match (&*g).allocate_nodes(n) {
+ Ok(_) => true,
+ Err(_) => false,
+ }
}
unsafe extern "C" fn Graph_AllocateEdges(
g: *mut Graph,
n: usize,
-) {
+) -> bool {
+ if g.is_null() {
+ return false;
+ }
- (&*g).allocate_edges(n);
+ match (&*g).allocate_edges(n) {
+ Ok(_) => true,
+ Err(_) => false,
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_GetAdjacencyMatrix(g: *mut Graph) -> *mut DeltaMatrix { | ||
| (&mut *g).get_adjacency_matrix() | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_GetLabelMatrix( | ||
| g: *mut Graph, | ||
| label: LabelID, | ||
| ) -> *mut DeltaMatrix { | ||
| (&mut *g).get_label_matrix(label) | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_GetRelationMatrix( | ||
| g: *mut Graph, | ||
| relation_idx: RelationID, | ||
| ) -> *mut DeltaMatrix { | ||
| (&mut *g).get_relation_matrix(relation_idx) | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_GetNodeLabelMatrix(g: *mut Graph) -> *mut DeltaMatrix { | ||
| (&mut *g).get_node_label_matrix() | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_GetZeroMatrix(g: *mut Graph) -> *mut DeltaMatrix { | ||
| (&mut *g).get_zero_matrix() | ||
| } |
There was a problem hiding this comment.
Add error handling for matrix retrieval functions.
The matrix retrieval functions don't handle potential null returns from the underlying operations.
Consider adding error handling:
unsafe extern "C" fn Graph_GetAdjacencyMatrix(
g: *mut Graph,
-) -> *mut DeltaMatrix {
+) -> Option<*mut DeltaMatrix> {
+ if g.is_null() {
+ return None;
+ }
- (&mut *g).get_adjacency_matrix()
+ Some((&mut *g).get_adjacency_matrix())
}Apply similar changes to other matrix retrieval functions.
Committable suggestion skipped: line range outside the PR's diff.
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_LabelNode( | ||
| g: *mut Graph, | ||
| id: NodeID, | ||
| lbls: *mut LabelID, | ||
| lbl_count: u32, | ||
| ) { | ||
| (&mut *g).label_node(id, from_raw_parts(lbls, lbl_count as usize)); | ||
| } |
There was a problem hiding this comment.
Add integer overflow protection for label count.
Converting lbl_count to usize without checking for overflow could lead to memory safety issues.
Add overflow checks:
unsafe extern "C" fn Graph_LabelNode(
g: *mut Graph,
id: NodeID,
lbls: *mut LabelID,
lbl_count: u32,
) {
+ let count = usize::try_from(lbl_count).expect("Label count overflow");
+ if lbl_count > 0 {
+ assert!(!lbls.is_null(), "Label pointer is null");
+ }
- (&mut *g).label_node(id, from_raw_parts(lbls, lbl_count as usize));
+ (&mut *g).label_node(id, from_raw_parts(lbls, count));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_LabelNode( | |
| g: *mut Graph, | |
| id: NodeID, | |
| lbls: *mut LabelID, | |
| lbl_count: u32, | |
| ) { | |
| (&mut *g).label_node(id, from_raw_parts(lbls, lbl_count as usize)); | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_LabelNode( | |
| g: *mut Graph, | |
| id: NodeID, | |
| lbls: *mut LabelID, | |
| lbl_count: u32, | |
| ) { | |
| let count = usize::try_from(lbl_count).expect("Label count overflow"); | |
| if lbl_count > 0 { | |
| assert!(!lbls.is_null(), "Label pointer is null"); | |
| } | |
| (&mut *g).label_node(id, from_raw_parts(lbls, count)); | |
| } |
src/graph/ffi.rs
Outdated
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_OptimizedFormConnections( | ||
| g: *mut Graph, | ||
| r: RelationID, | ||
| srcs: *const NodeID, | ||
| dests: *const NodeID, | ||
| ids: *const EdgeID, | ||
| n: u64, | ||
| multi_edge: bool, | ||
| ) { | ||
| (&mut *g).optimized_form_connections( | ||
| r, | ||
| from_raw_parts(srcs, n as usize), | ||
| from_raw_parts(dests, n as usize), | ||
| from_raw_parts(ids, n as usize), | ||
| multi_edge, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add array length validation for optimized connections.
The function uses multiple arrays that must have the same length, but there's no validation to ensure this.
Add length validation:
unsafe extern "C" fn Graph_OptimizedFormConnections(
g: *mut Graph,
r: RelationID,
srcs: *const NodeID,
dests: *const NodeID,
ids: *const EdgeID,
n: u64,
multi_edge: bool,
) {
+ if g.is_null() || srcs.is_null() || dests.is_null() || ids.is_null() {
+ return;
+ }
+ let count = usize::try_from(n).expect("Length overflow");
(&mut *g).optimized_form_connections(
r,
- from_raw_parts(srcs, n as usize),
- from_raw_parts(dests, n as usize),
- from_raw_parts(ids, n as usize),
+ from_raw_parts(srcs, count),
+ from_raw_parts(dests, count),
+ from_raw_parts(ids, count),
multi_edge,
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_OptimizedFormConnections( | |
| g: *mut Graph, | |
| r: RelationID, | |
| srcs: *const NodeID, | |
| dests: *const NodeID, | |
| ids: *const EdgeID, | |
| n: u64, | |
| multi_edge: bool, | |
| ) { | |
| (&mut *g).optimized_form_connections( | |
| r, | |
| from_raw_parts(srcs, n as usize), | |
| from_raw_parts(dests, n as usize), | |
| from_raw_parts(ids, n as usize), | |
| multi_edge, | |
| ); | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_OptimizedFormConnections( | |
| g: *mut Graph, | |
| r: RelationID, | |
| srcs: *const NodeID, | |
| dests: *const NodeID, | |
| ids: *const EdgeID, | |
| n: u64, | |
| multi_edge: bool, | |
| ) { | |
| if g.is_null() || srcs.is_null() || dests.is_null() || ids.is_null() { | |
| return; | |
| } | |
| let count = usize::try_from(n).expect("Length overflow"); | |
| (&mut *g).optimized_form_connections( | |
| r, | |
| from_raw_parts(srcs, count), | |
| from_raw_parts(dests, count), | |
| from_raw_parts(ids, count), | |
| multi_edge, | |
| ); | |
| } |
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_NodeCount(g: *mut Graph) -> u64 { | ||
| (&*g).node_count() | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_DeletedNodeCount(g: *mut Graph) -> u64 { | ||
| (&*g).deleted_node_count() | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_UncompactedNodeCount(g: *mut Graph) -> u64 { | ||
| (&*g).uncompacted_node_count() | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_LabeledNodeCount( | ||
| g: *mut Graph, | ||
| label: i32, | ||
| ) -> u64 { | ||
| (&*g).labeled_node_count(label) | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_EdgeCount(g: *mut Graph) -> u64 { | ||
| (&*g).edge_count() | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_RelationEdgeCount( | ||
| g: *mut Graph, | ||
| relation_idx: i32, | ||
| ) -> u64 { | ||
| (&*g).relation_edge_count(relation_idx) | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_DeletedEdgeCount(g: *mut Graph) -> u32 { | ||
| (&*g).deleted_edge_count() | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_RelationTypeCount(g: *mut Graph) -> i32 { | ||
| (&*g).relation_type_count() | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_LabelTypeCount(g: *mut Graph) -> i32 { | ||
| (&*g).label_type_count() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize return types for count functions.
The count functions use inconsistent return types (u64, u32, i32), which could lead to confusion and potential overflow issues.
Consider standardizing return types:
-unsafe extern "C" fn Graph_DeletedEdgeCount(g: *mut Graph) -> u32 {
+unsafe extern "C" fn Graph_DeletedEdgeCount(g: *mut Graph) -> u64 {
- (&*g).deleted_edge_count()
+ (&*g).deleted_edge_count() as u64
}
-unsafe extern "C" fn Graph_RelationTypeCount(g: *mut Graph) -> i32 {
+unsafe extern "C" fn Graph_RelationTypeCount(g: *mut Graph) -> u64 {
- (&*g).relation_type_count()
+ (&*g).relation_type_count() as u64
}
-unsafe extern "C" fn Graph_LabelTypeCount(g: *mut Graph) -> i32 {
+unsafe extern "C" fn Graph_LabelTypeCount(g: *mut Graph) -> u64 {
- (&*g).label_type_count()
+ (&*g).label_type_count() as u64
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_NodeCount(g: *mut Graph) -> u64 { | |
| (&*g).node_count() | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_DeletedNodeCount(g: *mut Graph) -> u64 { | |
| (&*g).deleted_node_count() | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_UncompactedNodeCount(g: *mut Graph) -> u64 { | |
| (&*g).uncompacted_node_count() | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_LabeledNodeCount( | |
| g: *mut Graph, | |
| label: i32, | |
| ) -> u64 { | |
| (&*g).labeled_node_count(label) | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_EdgeCount(g: *mut Graph) -> u64 { | |
| (&*g).edge_count() | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_RelationEdgeCount( | |
| g: *mut Graph, | |
| relation_idx: i32, | |
| ) -> u64 { | |
| (&*g).relation_edge_count(relation_idx) | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_DeletedEdgeCount(g: *mut Graph) -> u32 { | |
| (&*g).deleted_edge_count() | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_RelationTypeCount(g: *mut Graph) -> i32 { | |
| (&*g).relation_type_count() | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_LabelTypeCount(g: *mut Graph) -> i32 { | |
| (&*g).label_type_count() | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_NodeCount(g: *mut Graph) -> u64 { | |
| (&*g).node_count() | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_DeletedNodeCount(g: *mut Graph) -> u64 { | |
| (&*g).deleted_node_count() | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_UncompactedNodeCount(g: *mut Graph) -> u64 { | |
| (&*g).uncompacted_node_count() | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_LabeledNodeCount( | |
| g: *mut Graph, | |
| label: i32, | |
| ) -> u64 { | |
| (&*g).labeled_node_count(label) | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_EdgeCount(g: *mut Graph) -> u64 { | |
| (&*g).edge_count() | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_RelationEdgeCount( | |
| g: *mut Graph, | |
| relation_idx: i32, | |
| ) -> u64 { | |
| (&*g).relation_edge_count(relation_idx) | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_DeletedEdgeCount(g: *mut Graph) -> u64 { | |
| (&*g).deleted_edge_count() as u64 | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_RelationTypeCount(g: *mut Graph) -> u64 { | |
| (&*g).relation_type_count() as u64 | |
| } | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn Graph_LabelTypeCount(g: *mut Graph) -> u64 { | |
| (&*g).label_type_count() as u64 | |
| } |
| unsafe { | ||
| self.edge_id = GxB_Vector_Iterator_getIndex(self.eit.as_mut().unwrap()); | ||
| let info = GxB_Vector_Iterator_next(self.eit.as_mut().unwrap()); | ||
| debug_assert!(info == GrB_Info::GrB_SUCCESS); |
There was a problem hiding this comment.
Replace debug assertions with proper error handling.
Debug assertions are used to check GraphBLAS operation results. These should be replaced with proper error handling as they will be stripped in release builds:
- debug_assert!(info == GrB_Info::GrB_SUCCESS);
+ if info != GrB_Info::GrB_SUCCESS {
+ // Handle error appropriately, e.g.:
+ log::error!("GraphBLAS iterator operation failed: {:?}", info);
+ return None;
+ }Also applies to: 516-516
| fn syncronize( | ||
| matrix_policy: MatrixPolicy, | ||
| m: &mut DeltaMatrix, | ||
| nrows: u64, | ||
| ncols: u64, | ||
| ) -> &mut DeltaMatrix { | ||
| match matrix_policy { | ||
| MatrixPolicy::FlushResize => m.synchronize(nrows, ncols), | ||
| MatrixPolicy::Resize => m.resize(nrows, ncols), | ||
| _ => (), | ||
| }; | ||
| m | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for matrix operations in synchronize
The synchronize method performs matrix operations without error handling. These operations could fail, especially in low-memory conditions.
fn syncronize(
matrix_policy: MatrixPolicy,
m: &mut DeltaMatrix,
nrows: u64,
ncols: u64,
-) -> &mut DeltaMatrix {
+) -> Result<&mut DeltaMatrix, &'static str> {
match matrix_policy {
- MatrixPolicy::FlushResize => m.synchronize(nrows, ncols),
- MatrixPolicy::Resize => m.resize(nrows, ncols),
+ MatrixPolicy::FlushResize => m.synchronize(nrows, ncols)
+ .map_err(|_| "Failed to synchronize matrix")?,
+ MatrixPolicy::Resize => m.resize(nrows, ncols)
+ .map_err(|_| "Failed to resize matrix")?,
_ => (),
};
- m
+ Ok(m)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn syncronize( | |
| matrix_policy: MatrixPolicy, | |
| m: &mut DeltaMatrix, | |
| nrows: u64, | |
| ncols: u64, | |
| ) -> &mut DeltaMatrix { | |
| match matrix_policy { | |
| MatrixPolicy::FlushResize => m.synchronize(nrows, ncols), | |
| MatrixPolicy::Resize => m.resize(nrows, ncols), | |
| _ => (), | |
| }; | |
| m | |
| } | |
| fn syncronize( | |
| matrix_policy: MatrixPolicy, | |
| m: &mut DeltaMatrix, | |
| nrows: u64, | |
| ncols: u64, | |
| ) -> Result<&mut DeltaMatrix, &'static str> { | |
| match matrix_policy { | |
| MatrixPolicy::FlushResize => m.synchronize(nrows, ncols) | |
| .map_err(|_| "Failed to synchronize matrix")?, | |
| MatrixPolicy::Resize => m.resize(nrows, ncols) | |
| .map_err(|_| "Failed to resize matrix")?, | |
| _ => (), | |
| }; | |
| Ok(m) | |
| } |
Implement CRWLock and CMutex on top of parking_lot.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (3)
src/binding/crwlock.rs (1)
16-20: Document deadlock prevention strategyThe struct needs documentation explaining how it prevents deadlocks, especially when acquiring multiple locks.
Add documentation like:
/// CRWLock provides read-write locking for graph operations. /// /// # Deadlock Prevention /// /// To prevent deadlocks: /// 1. Locks are always acquired in a consistent order /// 2. No recursive locking is allowed /// 3. Lock timeout mechanism is implemented pub struct CRWLock { pub rwlock: RwLock<()>, }src/graph/matrix/delta_matrix.rs (2)
Line range hint
277-302: Consider using RAII for GraphBLAS resourcesThe scalar allocation and deallocation could leak resources on panic. Consider using RAII:
struct GrBScalar { scalar: GrB_Scalar, } impl Drop for GrBScalar { fn drop(&mut self) { unsafe { GrB_Scalar_free(&mut self.scalar); } } }
Line range hint
489-501: Add documentation for synchronization strategyThe sync method's logic is complex and needs documentation explaining the strategy.
Add documentation like:
/// Synchronizes the matrix state based on conditions: /// 1. force_sync: Immediately applies all pending changes /// 2. delta_max_pending_changes: Applies changes when deltas exceed threshold /// /// # Arguments /// * force_sync - Force immediate synchronization /// * delta_max_pending_changes - Maximum pending changes before auto-sync fn sync(&mut self, force_sync: bool, delta_max_pending_changes: u64)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
Cargo.toml(1 hunks)src/binding/crwlock.rs(1 hunks)src/binding/mod.rs(1 hunks)src/graph/graph.rs(1 hunks)src/graph/matrix/delta_matrix.rs(32 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/binding/mod.rs
🔇 Additional comments (7)
Cargo.toml (1)
14-14: Consider using the latest stable version of parking_lot
The current version (0.12.3) seems to be ahead of the latest stable release. Consider using version 0.12.1 unless there's a specific requirement for 0.12.3.
src/binding/crwlock.rs (3)
31-37:
Add error handling for lock acquisition
The acquire_read method should handle potential lock acquisition failures.
39-45:
Add error handling for write lock acquisition
The acquire_write method should handle potential lock acquisition failures.
47-51:
Add error handling for lock release
The release method should handle potential unlock failures.
src/graph/matrix/delta_matrix.rs (2)
86-90:
Handle None case in transpose access
The unwrap on transpose access could panic. Consider proper error handling.
338-342:
Improve error handling in mxm method
The transpose access could panic if None. Consider proper error handling:
- let n = if desc == GrB_DESC_T1 && n.transpose.is_some() {
- n.transpose.as_ref().unwrap()
- } else {
- &n.matrix
- };
+ let n = match (desc == GrB_DESC_T1, &n.transpose) {
+ (true, Some(t)) => t,
+ _ => &n.matrix
+ };Likely invalid or redundant comment.
src/graph/graph.rs (1)
944-964:
Fix potential stack overflow and add error handling in EdgeIterator::next
The current implementation has several issues:
- Uses recursion that could lead to stack overflow with many relation types
- No error handling for get_edge
- Missing validation of retrieved edge data
Consider this safer implementation:
-pub fn next(&mut self, e: &mut Edge) -> bool {
+pub fn next(&mut self, e: &mut Edge) -> Result<bool, &'static str> {
- if let Some((src_id, dest_id, edge_id)) = self.it.next() {
- e.src_id = src_id;
- e.dest_id = dest_id;
- e.relation_id = self.r;
- self.graph.get_edge(edge_id, e);
- return true;
- }
-
- if self.all_edges && self.r + 1 < self.graph.relation_type_count() {
- self.r += 1;
- self.graph.get_relation_matrix(self.r);
- self.it = self.graph.relations[self.r as usize].iter(self.src_id, self.dest_id);
- return self.next(e);
- }
- false
+ loop {
+ if let Some((src_id, dest_id, edge_id)) = self.it.next() {
+ e.src_id = src_id;
+ e.dest_id = dest_id;
+ e.relation_id = self.r;
+ if !self.graph.get_edge(edge_id, e) {
+ return Err("Failed to get edge");
+ }
+ return Ok(true);
+ }
+
+ if !self.all_edges || self.r + 1 >= self.graph.relation_type_count() {
+ return Ok(false);
+ }
+
+ self.r += 1;
+ self.graph.get_relation_matrix(self.r);
+ self.it = self.graph.relations[self.r as usize].iter(self.src_id, self.dest_id);
+ }
}Likely invalid or redundant comment.
| pub fn export( | ||
| &self, | ||
| transpose: bool, | ||
| ) -> SparseMatrix { | ||
| let s = if transpose { | ||
| self.transpose.as_ref().unwrap() | ||
| } else { | ||
| m.transpose(None, null_mut(), Some(&self.matrix), unsafe { | ||
| GrB_DESC_RT0 | ||
| &self.matrix | ||
| }; |
There was a problem hiding this comment.
Add error handling to export method
The export method could panic on transpose access.
- let s = if transpose {
- self.transpose.as_ref().unwrap()
- } else {
- &self.matrix
- };
+ let s = match (transpose, &self.transpose) {
+ (true, Some(t)) => t,
+ (true, None) => return Err(Error::NoTranspose),
+ (false, _) => &self.matrix
+ };Committable suggestion skipped: line range outside the PR's diff.
| pub fn next(&mut self) -> Option<Edge> { | ||
| if self.depleted { | ||
| return None; | ||
| } | ||
| loop { | ||
| if let Some((src_id, dest_id, edge_id, _)) = self.it.next() { | ||
| let mut e = Edge { | ||
| src_id: src_id, | ||
| dest_id: dest_id, | ||
| id: 0, | ||
| attributes: null_mut(), | ||
| relation_id: self.r, | ||
| relationship: null_mut(), | ||
| }; | ||
| self.graph.get_edge(edge_id, &mut e); | ||
| return Some(e); | ||
| } | ||
| if self.both_dir && self.dir == GraphEdgeDir::Outgoing { | ||
| self.dir = GraphEdgeDir::Incoming; | ||
| self.it = self.graph.relations[self.r as usize].iter_range( | ||
| self.node_id, | ||
| self.node_id, | ||
| true, | ||
| ); | ||
| continue; | ||
| } | ||
| return None; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling and improve safety in NodeEdgeIterator::next
The current implementation has several issues:
- No error handling for get_edge
- Returns edge with potentially null attributes
- Complex control flow could be simplified
Consider this safer implementation:
-pub fn next(&mut self) -> Option<Edge> {
+pub fn next(&mut self) -> Result<Option<Edge>, &'static str> {
if self.depleted {
- return None;
+ return Ok(None);
}
loop {
if let Some((src_id, dest_id, edge_id, _)) = self.it.next() {
let mut e = Edge {
src_id,
dest_id,
id: 0,
attributes: null_mut(),
relation_id: self.r,
relationship: null_mut(),
};
- self.graph.get_edge(edge_id, &mut e);
- return Some(e);
+ if !self.graph.get_edge(edge_id, &mut e) {
+ return Err("Failed to get edge");
+ }
+ if e.attributes.is_null() {
+ return Err("Edge has null attributes");
+ }
+ return Ok(Some(e));
}
if self.both_dir && self.dir == GraphEdgeDir::Outgoing {
self.dir = GraphEdgeDir::Incoming;
self.it = self.graph.relations[self.r as usize].iter_range(
self.node_id,
self.node_id,
true,
);
continue;
}
- return None;
+ return Ok(None);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn next(&mut self) -> Option<Edge> { | |
| if self.depleted { | |
| return None; | |
| } | |
| loop { | |
| if let Some((src_id, dest_id, edge_id, _)) = self.it.next() { | |
| let mut e = Edge { | |
| src_id: src_id, | |
| dest_id: dest_id, | |
| id: 0, | |
| attributes: null_mut(), | |
| relation_id: self.r, | |
| relationship: null_mut(), | |
| }; | |
| self.graph.get_edge(edge_id, &mut e); | |
| return Some(e); | |
| } | |
| if self.both_dir && self.dir == GraphEdgeDir::Outgoing { | |
| self.dir = GraphEdgeDir::Incoming; | |
| self.it = self.graph.relations[self.r as usize].iter_range( | |
| self.node_id, | |
| self.node_id, | |
| true, | |
| ); | |
| continue; | |
| } | |
| return None; | |
| } | |
| } | |
| pub fn next(&mut self) -> Result<Option<Edge>, &'static str> { | |
| if self.depleted { | |
| return Ok(None); | |
| } | |
| loop { | |
| if let Some((src_id, dest_id, edge_id, _)) = self.it.next() { | |
| let mut e = Edge { | |
| src_id, | |
| dest_id, | |
| id: 0, | |
| attributes: null_mut(), | |
| relation_id: self.r, | |
| relationship: null_mut(), | |
| }; | |
| if !self.graph.get_edge(edge_id, &mut e) { | |
| return Err("Failed to get edge"); | |
| } | |
| if e.attributes.is_null() { | |
| return Err("Edge has null attributes"); | |
| } | |
| return Ok(Some(e)); | |
| } | |
| if self.both_dir && self.dir == GraphEdgeDir::Outgoing { | |
| self.dir = GraphEdgeDir::Incoming; | |
| self.it = self.graph.relations[self.r as usize].iter_range( | |
| self.node_id, | |
| self.node_id, | |
| true, | |
| ); | |
| continue; | |
| } | |
| return Ok(None); | |
| } | |
| } |
| impl Drop for Graph { | ||
| fn drop(&mut self) { | ||
| unsafe { | ||
| let it = if self.partial { | ||
| DataBlock_FullScan(self.nodes) | ||
| } else { | ||
| self.scan_nodes() | ||
| }; | ||
| loop { | ||
| let set = DataBlockIterator_Next(it, null_mut()) as *mut AttributeSet; | ||
| if set.is_null() { | ||
| break; | ||
| } | ||
| AttributeSet_Free(set); | ||
| } | ||
| DataBlockIterator_Free(it); | ||
|
|
||
| let it = if self.partial { | ||
| DataBlock_FullScan(self.edges) | ||
| } else { | ||
| self.scan_edges() | ||
| }; | ||
| loop { | ||
| let set = DataBlockIterator_Next(it, null_mut()) as *mut AttributeSet; | ||
| if set.is_null() { | ||
| break; | ||
| } | ||
| AttributeSet_Free(set); | ||
| } | ||
| DataBlockIterator_Free(it); | ||
|
|
||
| DataBlock_Free(self.nodes); | ||
| DataBlock_Free(self.edges); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve safety documentation and error handling in Drop implementation
The Drop implementation contains unsafe code without proper documentation and error handling.
Consider this improved implementation:
impl Drop for Graph {
fn drop(&mut self) {
unsafe {
+ // SAFETY: These FFI calls are safe because:
+ // 1. self.nodes and self.edges are valid pointers from Graph::new
+ // 2. DataBlock_FullScan/DataBlock_Scan return valid iterators
+ // 3. We properly free all resources in the correct order
+
+ // Free node attributes
let it = if self.partial {
DataBlock_FullScan(self.nodes)
} else {
self.scan_nodes()
};
+ if it.is_null() {
+ // Log error but continue cleanup
+ eprintln!("Warning: Failed to create node iterator during cleanup");
+ }
loop {
let set = DataBlockIterator_Next(it, null_mut()) as *mut AttributeSet;
if set.is_null() {
break;
}
+ // SAFETY: set is a valid pointer from DataBlockIterator_Next
AttributeSet_Free(set);
}
DataBlockIterator_Free(it);
// Similar improvements for edge cleanup...
+ // SAFETY: These are the original pointers from Graph::new
DataBlock_Free(self.nodes);
DataBlock_Free(self.edges);
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl Drop for Graph { | |
| fn drop(&mut self) { | |
| unsafe { | |
| let it = if self.partial { | |
| DataBlock_FullScan(self.nodes) | |
| } else { | |
| self.scan_nodes() | |
| }; | |
| loop { | |
| let set = DataBlockIterator_Next(it, null_mut()) as *mut AttributeSet; | |
| if set.is_null() { | |
| break; | |
| } | |
| AttributeSet_Free(set); | |
| } | |
| DataBlockIterator_Free(it); | |
| let it = if self.partial { | |
| DataBlock_FullScan(self.edges) | |
| } else { | |
| self.scan_edges() | |
| }; | |
| loop { | |
| let set = DataBlockIterator_Next(it, null_mut()) as *mut AttributeSet; | |
| if set.is_null() { | |
| break; | |
| } | |
| AttributeSet_Free(set); | |
| } | |
| DataBlockIterator_Free(it); | |
| DataBlock_Free(self.nodes); | |
| DataBlock_Free(self.edges); | |
| } | |
| } | |
| } | |
| impl Drop for Graph { | |
| fn drop(&mut self) { | |
| unsafe { | |
| // SAFETY: These FFI calls are safe because: | |
| // 1. self.nodes and self.edges are valid pointers from Graph::new | |
| // 2. DataBlock_FullScan/DataBlock_Scan return valid iterators | |
| // 3. We properly free all resources in the correct order | |
| // Free node attributes | |
| let it = if self.partial { | |
| DataBlock_FullScan(self.nodes) | |
| } else { | |
| self.scan_nodes() | |
| }; | |
| if it.is_null() { | |
| // Log error but continue cleanup | |
| eprintln!("Warning: Failed to create node iterator during cleanup"); | |
| } | |
| loop { | |
| let set = DataBlockIterator_Next(it, null_mut()) as *mut AttributeSet; | |
| if set.is_null() { | |
| break; | |
| } | |
| // SAFETY: set is a valid pointer from DataBlockIterator_Next | |
| AttributeSet_Free(set); | |
| } | |
| DataBlockIterator_Free(it); | |
| let it = if self.partial { | |
| DataBlock_FullScan(self.edges) | |
| } else { | |
| self.scan_edges() | |
| }; | |
| loop { | |
| let set = DataBlockIterator_Next(it, null_mut()) as *mut AttributeSet; | |
| if set.is_null() { | |
| break; | |
| } | |
| AttributeSet_Free(set); | |
| } | |
| DataBlockIterator_Free(it); | |
| // SAFETY: These are the original pointers from Graph::new | |
| DataBlock_Free(self.nodes); | |
| DataBlock_Free(self.edges); | |
| } | |
| } | |
| } |
src/binding/crwlock.rs
Outdated
| thread_local! { | ||
| static READ_GUARD: Cell<Option<RwLockReadGuard<'static, ()>>> = Cell::new(None); | ||
| static WRITE_GUARD: Cell<Option<RwLockWriteGuard<'static, ()>>> = Cell::new(None); | ||
| } |
There was a problem hiding this comment.
Consider potential memory leaks with thread_local storage.
The thread_local storage of lock guards could lead to memory leaks if threads terminate without calling release(). Consider using RAII patterns with a guard type that implements Drop to ensure automatic cleanup.
pub struct CRWLockGuard<'a> {
lock: &'a CRWLock,
}
impl<'a> Drop for CRWLockGuard<'a> {
fn drop(&mut self) {
self.lock.release();
}
}| unsafe impl Sync for CRWLock {} | ||
|
|
||
| impl CRWLock { | ||
| pub fn new() -> Self { | ||
| CRWLock { |
There was a problem hiding this comment.
Add deadlock prevention mechanism.
The current implementation lacks deadlock prevention. If a thread panics while holding a lock, other threads may deadlock.
impl CRWLock {
pub fn acquire_read(&self) -> Result<CRWLockGuard, &'static str> {
match catch_unwind(|| self.rwlock.read()) {
Ok(guard) => {
READ_GUARD.with(|cell|
cell.set(Some(unsafe { std::mem::transmute(guard) })));
Ok(CRWLockGuard { lock: self })
}
Err(_) => Err("Thread panicked while acquiring read lock")
}
}
}
src/binding/crwlock.rs
Outdated
| unsafe { | ||
| READ_GUARD.set(Some(std::mem::transmute(guard))); | ||
| } |
There was a problem hiding this comment.
Unsafe lifetime transmute needs justification.
The transmute of lock guard lifetimes is dangerous and could lead to use-after-free bugs.
Consider using a safer approach:
thread_local! {
static READ_GUARD: RefCell<Option<RwLockReadGuard<'static, ()>>> = RefCell::new(None);
}
src/binding/crwlock.rs
Outdated
| #[inline] | ||
| pub fn release(&self) { | ||
| WRITE_GUARD.take(); | ||
| READ_GUARD.take(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for release operation.
The release method should handle potential errors that may occur during unlock operations.
pub fn release(&self) -> Result<(), &'static str> {
READ_GUARD.with(|cell| cell.take());
WRITE_GUARD.with(|cell| cell.take());
Ok(())
}
src/graph/ffi.rs
Outdated
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_AcquireReadLock(g: *mut Graph) { | ||
| (&mut *g).acquire_read_lock(); | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_AcquireWriteLock(g: *mut Graph) { | ||
| (&mut *g).acquire_write_lock(); | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_ReleaseLock(g: *mut Graph) { | ||
| (&mut *g).release_lock(); | ||
| } |
There was a problem hiding this comment.
Add null pointer checks and error handling in lock management functions.
The lock management functions lack null pointer checks and error handling.
unsafe extern "C" fn Graph_AcquireReadLock(g: *mut Graph) -> bool {
if g.is_null() {
return false;
}
match (&mut *g).acquire_read_lock() {
Ok(_) => true,
Err(_) => false
}
}| pub fn get_node_labels( | ||
| &mut self, | ||
| n: &Node, | ||
| labels: &mut [i32], | ||
| ) -> u32 { | ||
| let m = self.get_node_label_matrix(); | ||
|
|
||
| let mut it = DeltaMatrixIter::new_range(m, n.id, n.id, false); | ||
| let mut i = 0; | ||
|
|
||
| while let Ok(Some((_, j))) = it.next_bool() { | ||
| labels[i as usize] = j as i32; | ||
| i += 1; | ||
| } | ||
|
|
||
| i | ||
| } |
There was a problem hiding this comment.
Add bounds checking in get_node_labels.
The function writes to labels slice without checking its capacity.
pub fn get_node_labels(&mut self, n: &Node, labels: &mut [i32]) -> Result<u32, &'static str> {
let m = self.get_node_label_matrix();
let mut it = DeltaMatrixIter::new_range(m, n.id, n.id, false);
let mut i = 0u32;
while let Ok(Some((_, j))) = it.next_bool() {
if (i as usize) >= labels.len() {
return Err("Labels buffer too small");
}
labels[i as usize] = j as i32;
i += 1;
}
Ok(i)
}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
src/graph/graph.rs (4)
6-38: Consider adding documentation for imported types and modulesThe imports are well-organized but lack documentation comments explaining the purpose of key types and modules, particularly the custom ones like
CRWLock,AttributeSet, etc. This would help developers understand the codebase better.
42-49: Add documentation for public enum MatrixPolicyThis public enum lacks documentation explaining its purpose and when each variant should be used. Consider adding doc comments.
Add documentation like this:
/// Policy for matrix operations in the graph. #[repr(C)] #[allow(dead_code)] #[derive(Clone, Copy)] pub enum MatrixPolicy { /// Flush pending changes and resize the matrix FlushResize, /// Only resize the matrix Resize, /// No operation Nop, }
105-119: Add documentation for Graph struct fieldsThe Graph struct lacks documentation explaining the purpose of each field and their invariants.
Add documentation like this:
/// Represents a graph data structure with nodes, edges, and their relationships. pub struct Graph { /// Number of nodes reserved but not yet created reserved_node_count: i32, /// Storage for node data nodes: *mut DataBlock, /// Storage for edge data edges: *mut DataBlock, // ... document other fields }
1044-1228: Enhance test coverage for error casesWhile the tests cover basic functionality, they don't test error cases such as:
- Failed allocations
- Integer overflows
- Invalid inputs
Consider adding tests for error cases:
#[test] fn test_create_node_error_cases() { test_init(); let mut g = Graph::new(1, 1); g.acquire_write_lock(); // Test overflow for _ in 0..std::u64::MAX { let mut n = g.reserve_node(); if let Err(e) = g.create_node(&mut n, &[]) { assert_eq!(e, "Failed to allocate node attributes"); break; } } // Test invalid labels let mut n = g.reserve_node(); assert!(g.create_node(&mut n, &[std::i32::MAX]).is_err()); g.release_lock(); }
| struct GraphStatistics { | ||
| node_count: Vec<u64>, | ||
| edge_count: Vec<u64>, | ||
| } | ||
| impl GraphStatistics { | ||
| fn introduce_label(&mut self) { | ||
| self.node_count.push(0); | ||
| } | ||
|
|
||
| fn increment_node_count( | ||
| &mut self, | ||
| l: LabelID, | ||
| arg: u64, | ||
| ) { | ||
| self.node_count[l as usize] += arg; | ||
| } | ||
|
|
||
| fn decrement_node_count( | ||
| &mut self, | ||
| l: LabelID, | ||
| arg: u64, | ||
| ) { | ||
| self.node_count[l as usize] -= arg; | ||
| } | ||
|
|
||
| fn introduce_relationship(&mut self) { | ||
| self.edge_count.push(0); | ||
| } | ||
|
|
||
| fn increment_edge_count( | ||
| &mut self, | ||
| r: RelationID, | ||
| arg: u64, | ||
| ) { | ||
| self.edge_count[r as usize] += arg; | ||
| } | ||
|
|
||
| fn decrement_edge_count( | ||
| &mut self, | ||
| r: RelationID, | ||
| arg: u64, | ||
| ) { | ||
| self.edge_count[r as usize] -= arg; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add bounds checking and overflow protection to GraphStatistics methods
The methods performing arithmetic operations on vectors lack bounds checking and overflow protection:
- Vector access without bounds checking could panic
- Arithmetic operations could overflow
Consider this safer implementation:
impl GraphStatistics {
fn increment_node_count(&mut self, l: LabelID, arg: u64) -> Result<(), &'static str> {
let count = self.node_count.get_mut(l as usize)
.ok_or("Invalid label ID")?;
*count = count.checked_add(arg)
.ok_or("Node count overflow")?;
Ok(())
}
fn decrement_node_count(&mut self, l: LabelID, arg: u64) -> Result<(), &'static str> {
let count = self.node_count.get_mut(l as usize)
.ok_or("Invalid label ID")?;
*count = count.checked_sub(arg)
.ok_or("Node count underflow")?;
Ok(())
}
}
src/graph/graph.rs
Outdated
| pub fn new( | ||
| node_cap: u64, | ||
| edge_cap: u64, | ||
| ) -> Self { | ||
| let nodes = unsafe { | ||
| DataBlock_New( | ||
| node_cap, | ||
| node_cap, | ||
| size_of::<AttributeSet>() as u32, | ||
| transmute_copy(&(AttributeSet_Free as *const c_void)), | ||
| ) | ||
| }; | ||
| let edges = unsafe { | ||
| DataBlock_New( | ||
| edge_cap, | ||
| edge_cap, | ||
| size_of::<AttributeSet>() as u32, | ||
| transmute_copy(&(AttributeSet_Free as *const c_void)), | ||
| ) | ||
| }; | ||
| Self { | ||
| reserved_node_count: 0, | ||
| nodes: nodes, | ||
| edges: edges, | ||
| adjacency_matrix: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, true), | ||
| labels: Vec::new(), | ||
| node_labels: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, false), | ||
| relations: Vec::new(), | ||
| zero_matrix: DeltaMatrix::new(unsafe { GrB_BOOL }, node_cap, node_cap, false), | ||
| crwlock: CRWLock::new(), | ||
| writelocked: false, | ||
| matrix_policy: MatrixPolicy::FlushResize, | ||
| stats: GraphStatistics { | ||
| node_count: Vec::new(), | ||
| edge_count: Vec::new(), | ||
| }, | ||
| partial: false, | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for FFI calls in Graph::new
The constructor uses unsafe FFI calls without checking for allocation failures.
Implement proper error handling:
pub fn new(node_cap: u64, edge_cap: u64) -> Result<Self, &'static str> {
let nodes = unsafe {
let ptr = DataBlock_New(
node_cap,
node_cap,
size_of::<AttributeSet>() as u32,
transmute_copy(&(AttributeSet_Free as *const c_void)),
);
if ptr.is_null() {
return Err("Failed to allocate nodes DataBlock");
}
ptr
};
let edges = unsafe {
let ptr = DataBlock_New(
edge_cap,
edge_cap,
size_of::<AttributeSet>() as u32,
transmute_copy(&(AttributeSet_Free as *const c_void)),
);
if ptr.is_null() {
// Clean up previously allocated resources
DataBlock_Free(nodes);
return Err("Failed to allocate edges DataBlock");
}
ptr
};
Ok(Self { /* ... */ })
}| pub fn create_node( | ||
| &mut self, | ||
| n: &mut Node, | ||
| labels: &[LabelID], | ||
| ) { | ||
| let id = n.id; | ||
| n.attributes = unsafe { DataBlock_AllocateItem(self.nodes, &mut n.id) } as _; | ||
| unsafe { n.attributes.write_unaligned(null_mut()) }; | ||
|
|
||
| if id != INVALID_ENTITY_ID { | ||
| debug_assert!(id == n.id); | ||
| self.reserved_node_count -= 1; | ||
| debug_assert!(self.reserved_node_count >= 0); | ||
| } | ||
|
|
||
| self.label_node(n.id, labels); | ||
| } |
There was a problem hiding this comment.
Add error handling and safety checks to create_node
The method has several safety issues:
- No error handling for FFI calls
- Potential null pointer dereference
- Possible integer underflow
Consider this safer implementation:
pub fn create_node(&mut self, n: &mut Node, labels: &[LabelID]) -> Result<(), &'static str> {
let id = n.id;
n.attributes = unsafe {
let ptr = DataBlock_AllocateItem(self.nodes, &mut n.id) as *mut AttributeSet;
if ptr.is_null() {
return Err("Failed to allocate node attributes");
}
ptr
};
if id != INVALID_ENTITY_ID {
debug_assert!(id == n.id);
self.reserved_node_count = self.reserved_node_count.checked_sub(1)
.ok_or("Reserved node count underflow")?;
}
self.label_node(n.id, labels)?;
Ok(())
}| pub fn next( | ||
| &mut self, | ||
| e: &mut Edge, | ||
| ) -> bool { | ||
| if let Some((src_id, dest_id, edge_id)) = self.it.next() { | ||
| e.src_id = src_id; | ||
| e.dest_id = dest_id; | ||
| e.relation_id = self.r; | ||
| self.graph.get_edge(edge_id, e); | ||
| return true; | ||
| } | ||
|
|
||
| if self.all_edges && self.r + 1 < self.graph.relation_type_count() { | ||
| self.r += 1; | ||
| self.graph.get_relation_matrix(self.r); | ||
| self.it = self.graph.relations[self.r as usize].iter(self.src_id, self.dest_id); | ||
| return self.next(e); | ||
| } | ||
| false | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix potential stack overflow in EdgeIterator::next
The recursive implementation could cause stack overflow with many relation types.
Replace recursion with iteration:
pub fn next(&mut self, e: &mut Edge) -> Result<bool, &'static str> {
loop {
if let Some((src_id, dest_id, edge_id)) = self.it.next() {
e.src_id = src_id;
e.dest_id = dest_id;
e.relation_id = self.r;
if !self.graph.get_edge(edge_id, e) {
return Err("Failed to get edge");
}
return Ok(true);
}
if !self.all_edges || self.r + 1 >= self.graph.relation_type_count() {
return Ok(false);
}
self.r += 1;
self.graph.get_relation_matrix(self.r);
self.it = self.graph.relations[self.r as usize].iter(self.src_id, self.dest_id);
}
}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
src/binding/crwlock.rs (2)
28-33:⚠️ Potential issueAdd error handling to lock initialization.
The
new()method should handle potential initialization failures.
43-49:⚠️ Potential issueFix unsafe lifetime transmute in acquire_write.
Similar issues as acquire_read:
- Unsafe transmute of lifetimes
- No error handling
- No deadlock prevention
src/graph/matrix/tensor.rs (3)
89-114:⚠️ Potential issueFix memory leak and add error handling in set_element.
The function has two critical issues:
- Memory leak: The vector
vis not freed in thesingle_edgebranch- Missing error handling for GraphBLAS operations
Apply this fix:
pub fn set_element(&mut self, src: u64, dest: u64, id: u64) { if let Some(current_edge) = self.m.extract_element_u64(src, dest) { unsafe { if single_edge(current_edge) { let mut v = MaybeUninit::uninit(); - GrB_Vector_new(v.as_mut_ptr(), GrB_BOOL, GrB_INDEX_MAX); + let info = GrB_Vector_new(v.as_mut_ptr(), GrB_BOOL, GrB_INDEX_MAX); + if info != GrB_Info::GrB_SUCCESS { + return; + } let v = v.assume_init(); self.m.set_element_u64(set_msb(v as u64), src, dest); GrB_Vector_setElement_BOOL(v, true, current_edge); GrB_Vector_setElement_BOOL(v, true, id); GrB_Vector_wait(v, GrB_WaitMode::GrB_MATERIALIZE); + GrB_Vector_free(&mut (v as *mut _)); } else {
465-496:⚠️ Potential issueImprove error handling in TensorIterator::next.
The iterator implementation has several issues:
- Uses unwrap() on Option which could panic
- Uses debug assertions which are stripped in release builds
- Lacks proper error handling for GraphBLAS operations
Apply this fix:
fn next(&mut self) -> Option<Self::Item> { if self.deleted { return None; } if single_edge(self.edge_id) { self.deleted = true; Some((self.src_id, self.dest_id, self.edge_id)) } else { if self.eit.is_none() { let v = clear_msb(self.edge_id) as GrB_Vector; let mut it = MaybeUninit::uninit(); unsafe { - GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut()); - GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0) + let info = GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut()); + if info != GrB_Info::GrB_SUCCESS { + self.deleted = true; + return None; + } + let info = GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0); + if info != GrB_Info::GrB_SUCCESS { + self.deleted = true; + return None; + } }; self.eit = Some(unsafe { it.assume_init() }); } unsafe { - let edge_id = GxB_Vector_Iterator_getIndex(self.eit.as_mut().unwrap()); - let info = GxB_Vector_Iterator_next(self.eit.as_mut().unwrap()); + let Some(it) = self.eit.as_mut() else { + return None; + }; + let edge_id = GxB_Vector_Iterator_getIndex(it); + let info = GxB_Vector_Iterator_next(it);
498-564:⚠️ Potential issueReplace debug assertions with proper error handling in TensorRangeIterator.
The implementation uses debug assertions which are stripped in release builds, potentially masking errors.
Apply this fix:
fn next(&mut self) -> Option<Self::Item> { if self.eit.is_some() { unsafe { - self.edge_id = GxB_Vector_Iterator_getIndex(self.eit.as_mut().unwrap()); - let info = GxB_Vector_Iterator_next(self.eit.as_mut().unwrap()); + let Some(it) = self.eit.as_mut() else { + return None; + }; + self.edge_id = GxB_Vector_Iterator_getIndex(it); + let info = GxB_Vector_Iterator_next(it); if info == GrB_Info::GxB_EXHAUSTED { self.eit = None; } return Some((self.src_id, self.dest_id, self.edge_id, true)); } } // ... rest of the implementation ... - debug_assert!(info == GrB_Info::GrB_SUCCESS); + if info != GrB_Info::GrB_SUCCESS { + self.eit = None; + return None; + }
🧹 Nitpick comments (2)
src/binding/crwlock.rs (1)
1-57: Consider architectural improvements for safer lock management.The current implementation could benefit from several architectural improvements:
- Use RAII pattern for automatic lock management
- Add proper error types and handling strategy
- Implement comprehensive testing strategy
- Add documentation for unsafe code and invariants
Consider creating these additional types and tests:
// Error handling #[derive(Debug)] pub enum LockError { Timeout, NotLocked, PoisonError, } // Tests #[cfg(test)] mod tests { use super::*; #[test] fn test_lock_guard_drop() { // Test RAII behavior } #[test] fn test_concurrent_access() { // Test thread safety } #[test] fn test_timeout_handling() { // Test lock timeout } }src/graph/matrix/tensor.rs (1)
27-37: Add documentation for bit manipulation functions.These utility functions are critical for edge ID manipulation but lack documentation explaining:
- The significance of the MSB flag
- When each function should be used
- The relationship between single edges and vectors
Add documentation like this:
+/// Returns true if the edge ID represents a single edge (MSB is 0). +/// Used to distinguish between single edges and edge vectors. pub fn single_edge(current_edge: EdgeID) -> bool { current_edge as u64 & (1u64 << (u64::BITS as usize - 1)) == 0 } +/// Sets the Most Significant Bit of the edge ID to mark it as a vector. pub fn set_msb(meid: EdgeID) -> u64 { meid as u64 | (1u64 << (u64::BITS as usize - 1)) } +/// Clears the Most Significant Bit of the edge ID to get the actual vector pointer. pub fn clear_msb(meid: EdgeID) -> u64 { meid as u64 & !(1u64 << (u64::BITS as usize - 1)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Cargo.toml(1 hunks)src/binding/crwlock.rs(1 hunks)src/graph/matrix/tensor.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🔇 Additional comments (1)
src/graph/matrix/tensor.rs (1)
1-564: Verify GraphBLAS resource management across the codebase.
Let's verify the usage of GraphBLAS resources to ensure proper cleanup.
✅ Verification successful
Based on the shell script results, I can now make a final assessment. The code shows a consistent pattern of GraphBLAS vector management:
GraphBLAS vector resources are properly managed
- All
GrB_Vector_newallocations are matched with correspondingGrB_Vector_freecalls - The Drop implementation for Tensor ensures cleanup of vector resources via
_free_vectorsunary operator - Vector resources are properly freed in error paths (e.g., when converting from single edge to vector representation)
- The implementation maintains RAII principles through Rust's ownership system
The codebase demonstrates proper resource management practices for GraphBLAS vectors, with no evidence of memory leaks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GraphBLAS vector allocations and their corresponding frees
echo "Checking for potential memory leaks in GraphBLAS vector operations..."
# Pattern 1: Vector allocations without corresponding frees
echo "Vector allocations:"
rg "GrB_Vector_new\(" -A 10
# Pattern 2: Vector frees
echo "Vector frees:"
rg "GrB_Vector_free\("
# Pattern 3: Potential error paths where cleanup might be missed
echo "Potential error paths:"
rg "if .* !=" -A 5
Length of output: 7529
src/binding/crwlock.rs
Outdated
| /// Wrap parking_lot rwlock to promote access from C and direct access from Rust. | ||
| /// Used to lock the graph. | ||
| pub struct CRWLock { | ||
| rwlock: RwLock<()>, | ||
| guard: ThreadLocal<Cell<Guard>>, | ||
| } | ||
| unsafe impl Send for CRWLock {} | ||
| unsafe impl Sync for CRWLock {} |
There was a problem hiding this comment.
Improve memory management and document unsafe implementations.
The current design has several issues:
- ThreadLocal storage of guards could lead to memory leaks if threads terminate without calling
release() - The unsafe Send/Sync implementations lack documentation explaining their safety guarantees
Consider these improvements:
- Use RAII pattern with a guard type:
pub struct CRWLockGuard<'a> {
lock: &'a CRWLock,
}
impl<'a> Drop for CRWLockGuard<'a> {
fn drop(&mut self) {
self.lock.release();
}
}- Document unsafe implementations:
/// Safety: The underlying RwLock is Send and the guard is managed thread-locally
unsafe impl Send for CRWLock {}
/// Safety: The underlying RwLock is Sync and the guard access is synchronized
unsafe impl Sync for CRWLock {}
src/binding/crwlock.rs
Outdated
| #[inline] | ||
| pub fn acquire_read(&self) { | ||
| let cell = self.guard.get_or(|| Cell::new(Guard::None)); | ||
| unsafe { | ||
| cell.set(Guard::Read(std::mem::transmute(self.rwlock.read()))); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix unsafe lifetime transmute in acquire_read.
The current implementation has several issues:
- Unsafe transmute of lifetimes is dangerous
- No error handling for lock acquisition
- No deadlock prevention mechanism
Consider this safer implementation:
pub fn acquire_read(&self) -> Result<CRWLockGuard, LockError> {
match self.rwlock.try_read_for(Duration::from_secs(1)) {
Some(guard) => {
let cell = self.guard.get_or(|| Cell::new(Guard::None));
cell.set(Guard::Read(guard));
Ok(CRWLockGuard { lock: self })
}
None => Err(LockError::Timeout),
}
}
src/binding/crwlock.rs
Outdated
| #[inline] | ||
| pub fn release(&self) { | ||
| if let Some(cell) = self.guard.get() { | ||
| cell.set(Guard::None); | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve release method safety and cleanup.
The current release implementation:
- Doesn't ensure proper cleanup of resources
- Doesn't handle potential errors during unlock
- Could leave the lock in an inconsistent state if the thread panics
Consider this safer implementation:
pub fn release(&self) -> Result<(), LockError> {
if let Some(cell) = self.guard.get() {
// Ensure guard is dropped properly
match cell.replace(Guard::None) {
Guard::None => Ok(()),
_ => {
// Clean up thread local storage
self.guard.get().map(|c| c.set(Guard::None));
Ok(())
}
}
} else {
Err(LockError::NotLocked)
}
}
src/binding/crwlock.rs
Outdated
| enum Guard { | ||
| Read(RwLockReadGuard<'static, ()>), | ||
| Write(RwLockWriteGuard<'static, ()>), | ||
| None, | ||
| } | ||
| unsafe impl Send for Guard {} | ||
|
|
There was a problem hiding this comment.
Remove unsafe static lifetimes from Guard enum.
The use of 'static lifetime for the guard references is dangerous as it bypasses Rust's lifetime checking mechanisms. This could lead to use-after-free bugs if the guard outlives the lock.
Consider using a generic lifetime parameter instead:
-enum Guard {
- Read(RwLockReadGuard<'static, ()>),
- Write(RwLockWriteGuard<'static, ()>),
- None,
-}
+enum Guard<'a> {
+ Read(RwLockReadGuard<'a, ()>),
+ Write(RwLockWriteGuard<'a, ()>),
+ None,
+}Additionally, document why the Send implementation is safe.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| enum Guard { | |
| Read(RwLockReadGuard<'static, ()>), | |
| Write(RwLockWriteGuard<'static, ()>), | |
| None, | |
| } | |
| unsafe impl Send for Guard {} | |
| enum Guard<'a> { | |
| Read(RwLockReadGuard<'a, ()>), | |
| Write(RwLockWriteGuard<'a, ()>), | |
| None, | |
| } | |
| unsafe impl Send for Guard {} |
src/graph/matrix/tensor.rs
Outdated
| static mut UNARYOP: GrB_UnaryOp = null_mut(); | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn _free_vectors( | ||
| z: *mut c_void, | ||
| x: *const c_void, | ||
| ) { | ||
| // see if entry is a vector | ||
| let x = *(x as *const EdgeID); | ||
| if !single_edge(x) { | ||
| let mut v = clear_msb(x) as GrB_Vector; | ||
| GrB_Vector_free(&mut v); | ||
| } | ||
| } | ||
|
|
||
| impl Drop for Tensor { | ||
| fn drop(&mut self) { | ||
| unsafe { | ||
| if UNARYOP == null_mut() { | ||
| grb_check!(GrB_UnaryOp_new( | ||
| &mut UNARYOP, | ||
| Some(_free_vectors), | ||
| GrB_UINT64, | ||
| GrB_UINT64 | ||
| )); | ||
| } | ||
|
|
||
| let m = self.m.m(false).grb_matrix_ref(); | ||
| grb_check!(GrB_Matrix_apply(m, null_mut(), null_mut(), UNARYOP, m, null_mut())); | ||
| let dp = self.m.dp(false).grb_matrix_ref(); | ||
| grb_check!(GrB_Matrix_apply(dp, null_mut(), null_mut(), UNARYOP, dp, null_mut())); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix potential race condition in UNARYOP initialization.
The static mutable UNARYOP could cause race conditions if multiple threads attempt to initialize it simultaneously. Consider using std::sync::Once for safe initialization.
Here's how to fix it:
-static mut UNARYOP: GrB_UnaryOp = null_mut();
+use std::sync::Once;
+static INIT: Once = Once::new();
+static mut UNARYOP: GrB_UnaryOp = null_mut();
impl Drop for Tensor {
fn drop(&mut self) {
unsafe {
- if UNARYOP == null_mut() {
+ INIT.call_once(|| {
grb_check!(GrB_UnaryOp_new(
&mut UNARYOP,
Some(_free_vectors),
GrB_UINT64,
GrB_UINT64
));
- }
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static mut UNARYOP: GrB_UnaryOp = null_mut(); | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn _free_vectors( | |
| z: *mut c_void, | |
| x: *const c_void, | |
| ) { | |
| // see if entry is a vector | |
| let x = *(x as *const EdgeID); | |
| if !single_edge(x) { | |
| let mut v = clear_msb(x) as GrB_Vector; | |
| GrB_Vector_free(&mut v); | |
| } | |
| } | |
| impl Drop for Tensor { | |
| fn drop(&mut self) { | |
| unsafe { | |
| if UNARYOP == null_mut() { | |
| grb_check!(GrB_UnaryOp_new( | |
| &mut UNARYOP, | |
| Some(_free_vectors), | |
| GrB_UINT64, | |
| GrB_UINT64 | |
| )); | |
| } | |
| let m = self.m.m(false).grb_matrix_ref(); | |
| grb_check!(GrB_Matrix_apply(m, null_mut(), null_mut(), UNARYOP, m, null_mut())); | |
| let dp = self.m.dp(false).grb_matrix_ref(); | |
| grb_check!(GrB_Matrix_apply(dp, null_mut(), null_mut(), UNARYOP, dp, null_mut())); | |
| } | |
| } | |
| } | |
| use std::sync::Once; | |
| static INIT: Once = Once::new(); | |
| static mut UNARYOP: GrB_UnaryOp = null_mut(); | |
| #[no_mangle] | |
| #[allow(non_snake_case)] | |
| unsafe extern "C" fn _free_vectors( | |
| z: *mut c_void, | |
| x: *const c_void, | |
| ) { | |
| // see if entry is a vector | |
| let x = *(x as *const EdgeID); | |
| if !single_edge(x) { | |
| let mut v = clear_msb(x) as GrB_Vector; | |
| GrB_Vector_free(&mut v); | |
| } | |
| } | |
| impl Drop for Tensor { | |
| fn drop(&mut self) { | |
| unsafe { | |
| INIT.call_once(|| { | |
| grb_check!(GrB_UnaryOp_new( | |
| &mut UNARYOP, | |
| Some(_free_vectors), | |
| GrB_UINT64, | |
| GrB_UINT64 | |
| )); | |
| }); | |
| let m = self.m.m(false).grb_matrix_ref(); | |
| grb_check!(GrB_Matrix_apply(m, null_mut(), null_mut(), UNARYOP, m, null_mut())); | |
| let dp = self.m.dp(false).grb_matrix_ref(); | |
| grb_check!(GrB_Matrix_apply(dp, null_mut(), null_mut(), UNARYOP, dp, null_mut())); | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
src/graph/matrix/tensor.rs (4)
95-114:⚠️ Potential issueFix memory leak in set_element.
The
GrB_Vectorcreated in theif single_edgebranch is never freed.Add vector cleanup:
GrB_Vector_setElement_BOOL(v, true, current_edge); GrB_Vector_setElement_BOOL(v, true, id); GrB_Vector_wait(v, GrB_WaitMode::GrB_MATERIALIZE); + GrB_Vector_free(&mut v); } else {
172-185:⚠️ Potential issueFree allocated GrB_Vector in set_elements.
The
GrB_Vectorallocated in the else branch is not freed after use.Add vector cleanup:
GrB_Vector_wait(v, GrB_WaitMode::GrB_MATERIALIZE); + GrB_Vector_free(&mut v); }
465-496:⚠️ Potential issueImprove safety in TensorIterator::next.
The iterator implementation has several safety concerns:
- No null pointer checks after iterator attachment
- Unchecked GraphBLAS operations
- Potential use-after-free when accessing the vector
Add safety checks:
fn next(&mut self) -> Option<Self::Item> { if self.deleted { return None; } if single_edge(self.edge_id) { self.deleted = true; Some((self.src_id, self.dest_id, self.edge_id)) } else { if self.eit.is_none() { let v = clear_msb(self.edge_id) as GrB_Vector; let mut it = MaybeUninit::uninit(); unsafe { - GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut()); - GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0) + if GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut()) != GrB_Info::GrB_SUCCESS { + self.deleted = true; + return None; + } + if GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0) != GrB_Info::GrB_SUCCESS { + self.deleted = true; + return None; + } }; self.eit = Some(unsafe { it.assume_init() }); } unsafe { + let Some(it) = self.eit.as_mut() else { + return None; + }; - let edge_id = GxB_Vector_Iterator_getIndex(self.eit.as_mut().unwrap()); - let info = GxB_Vector_Iterator_next(self.eit.as_mut().unwrap()); + let edge_id = GxB_Vector_Iterator_getIndex(it); + let info = GxB_Vector_Iterator_next(it); if info == GrB_Info::GxB_EXHAUSTED { self.eit = None; self.deleted = true; } return Some((self.src_id, self.dest_id, edge_id)); } } }
532-532:⚠️ Potential issueReplace debug assertions with proper error handling.
Debug assertions are used to check GraphBLAS operation results. These will be stripped in release builds.
- debug_assert!(info == GrB_Info::GrB_SUCCESS); + if info != GrB_Info::GrB_SUCCESS { + // Handle error appropriately, e.g.: + log::error!("GraphBLAS iterator operation failed: {:?}", info); + return None; + }src/graph/graph.rs (1)
121-158:⚠️ Potential issueAdd error handling for FFI calls in Graph::new.
The constructor uses unsafe FFI calls without checking for allocation failures.
Implement proper error handling:
- pub fn new(node_cap: u64, edge_cap: u64) -> Self { + pub fn new(node_cap: u64, edge_cap: u64) -> Result<Self, &'static str> { let nodes = unsafe { + // SAFETY: DataBlock_New is an FFI call that allocates memory. + // It's unsafe because it can return a null pointer if allocation fails. let ptr = DataBlock_New( node_cap, node_cap, size_of::<AttributeSet>() as u32, transmute_copy(&(AttributeSet_Free as *const c_void)), ); + if ptr.is_null() { + return Err("Failed to allocate nodes DataBlock"); + } ptr }; let edges = unsafe { + // SAFETY: DataBlock_New is an FFI call that allocates memory. + // It's unsafe because it can return a null pointer if allocation fails. let ptr = DataBlock_New( edge_cap, edge_cap, size_of::<AttributeSet>() as u32, transmute_copy(&(AttributeSet_Free as *const c_void)), ); + if ptr.is_null() { + // Clean up previously allocated resources + DataBlock_Free(nodes); + return Err("Failed to allocate edges DataBlock"); + } ptr }; - Self { + Ok(Self { reserved_node_count: 0, nodes, edges, // ... rest of the fields ... - } + }) }
🧹 Nitpick comments (1)
src/graph/matrix/tensor.rs (1)
27-37: Add documentation for bit manipulation functions.These utility functions manipulate edge IDs using bit operations, but their purpose and usage aren't immediately clear. Consider adding documentation to explain:
- The significance of the MSB in edge identification
- When and why these functions should be used
- The relationship between single edges and multi-edges
+/// Returns true if the edge ID represents a single edge (MSB is 0) pub fn single_edge(current_edge: EdgeID) -> bool { current_edge as u64 & (1u64 << (u64::BITS as usize - 1)) == 0 } +/// Sets the MSB of the edge ID to mark it as a multi-edge container pub fn set_msb(meid: EdgeID) -> u64 { meid as u64 | (1u64 << (u64::BITS as usize - 1)) } +/// Clears the MSB of the edge ID to get the actual vector pointer pub fn clear_msb(meid: EdgeID) -> u64 { meid as u64 & !(1u64 << (u64::BITS as usize - 1)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Cargo.toml(1 hunks)build.rs(2 hunks)src/binding/crwlock.rs(1 hunks)src/graph/ffi.rs(1 hunks)src/graph/graph.rs(1 hunks)src/graph/matrix/tensor.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🔇 Additional comments (4)
src/binding/crwlock.rs (2)
8-12:
Remove unsafe static lifetimes from guard variants.
The use of 'static lifetime for the guard references is unsafe as it bypasses Rust's lifetime checking mechanisms. This could lead to use-after-free bugs if the guard outlives the lock.
Apply this diff to use proper lifetime parameters:
-pub enum CRWGuard {
- Read(RwLockReadGuard<'static, ()>),
- Write(RwLockWriteGuard<'static, ()>),
-}
+pub enum CRWGuard<'a> {
+ Read(RwLockReadGuard<'a, ()>),
+ Write(RwLockWriteGuard<'a, ()>),
+}Likely invalid or redundant comment.
14-20: 🛠️ Refactor suggestion
Document safety guarantees for Send and Sync implementations.
The unsafe implementations of Send and Sync traits need documentation explaining why they are safe.
Add safety documentation:
/// Wrap parking_lot rwlock to promote access from C and direct access from Rust.
/// Used to lock the graph.
pub struct CRWLock {
rwlock: RwLock<()>,
}
+/// Safety: The underlying RwLock is Send and can be safely sent between threads
unsafe impl Send for CRWLock {}
+/// Safety: The underlying RwLock is Sync and can be safely shared between threads
unsafe impl Sync for CRWLock {}Likely invalid or redundant comment.
build.rs (1)
11-11: Verify LLVM 19 compatibility.
The addition of LLVM 19 library path needs verification to ensure compatibility with the project's requirements.
Also applies to: 26-26
✅ Verification successful
LLVM 19 library path addition is safe
The build script shows a progressive support for multiple LLVM versions (17, 18, and 19) when using Clang. The addition is consistent with the existing pattern and maintains backward compatibility since:
- LLVM paths are only added when
CLANG=1 - Multiple LLVM version paths are included (17, 18, 19), allowing flexibility in the environment
- The change only adds a library search path and doesn't enforce any specific LLVM version requirement
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for LLVM version compatibility issues
echo "Checking for LLVM version requirements..."
if [ -f "README.md" ]; then
rg -i "llvm.*version|version.*llvm" README.md
fi
Length of output: 163
Script:
#!/bin/bash
# Let's check for LLVM version mentions in source files and build scripts
echo "Checking source files for LLVM version requirements..."
rg -i "llvm.*(version|compatibility)" --type rust
# Check Cargo.toml for dependencies that might indicate LLVM version requirements
echo -e "\nChecking Cargo.toml for LLVM-related dependencies..."
if [ -f "Cargo.toml" ]; then
rg "llvm" Cargo.toml
fi
# Check for any LLVM-related configuration or version constraints
echo -e "\nChecking for LLVM configuration files..."
fd -t f ".*\.(toml|rs)" -X rg -l "llvm"
Length of output: 484
Script:
#!/bin/bash
# Let's examine the build.rs file to understand the LLVM usage context
echo "Checking build.rs for LLVM configuration..."
cat build.rs
# Also check for any existing LLVM library path configurations
echo -e "\nChecking for existing LLVM library configurations..."
rg -B2 -A2 "LLVM" build.rs
Length of output: 1464
src/graph/matrix/tensor.rs (1)
43-77:
Fix thread-safety issue with static mutable UNARYOP.
The static mutable UNARYOP is not thread-safe and could cause race conditions if multiple threads attempt to initialize it simultaneously.
Use std::sync::Once for safe initialization:
-static mut UNARYOP: GrB_UnaryOp = null_mut();
+use std::sync::Once;
+static INIT: Once = Once::new();
+static mut UNARYOP: GrB_UnaryOp = null_mut();
impl Drop for Tensor {
fn drop(&mut self) {
unsafe {
- if UNARYOP == null_mut() {
+ INIT.call_once(|| {
grb_check!(GrB_UnaryOp_new(
&mut UNARYOP,
Some(_free_vectors),
GrB_UINT64,
GrB_UINT64
));
- }
+ });Likely invalid or redundant comment.
| impl CRWLock { | ||
| pub fn new() -> Self { | ||
| CRWLock { | ||
| rwlock: RwLock::new(()), | ||
| } | ||
| } | ||
|
|
||
| #[inline] | ||
| pub fn acquire_read(&self) -> CRWGuard { | ||
| CRWGuard::Read(unsafe { std::mem::transmute(self.rwlock.read()) }) | ||
| } | ||
|
|
||
| #[inline] | ||
| pub fn acquire_write(&self) -> CRWGuard { | ||
| CRWGuard::Write(unsafe { std::mem::transmute(self.rwlock.write()) }) | ||
| } |
There was a problem hiding this comment.
Improve safety in lock operations.
The current implementation has several safety issues:
- Unsafe transmute of lifetimes is dangerous
- No error handling for lock acquisition
- No deadlock prevention mechanism
Consider this safer implementation:
impl CRWLock {
pub fn new() -> Self {
CRWLock {
rwlock: RwLock::new(()),
}
}
pub fn acquire_read(&self) -> Result<CRWGuard<'_>, LockError> {
match self.rwlock.try_read_for(Duration::from_secs(1)) {
Some(guard) => Ok(CRWGuard::Read(guard)),
None => Err(LockError::Timeout),
}
}
pub fn acquire_write(&self) -> Result<CRWGuard<'_>, LockError> {
match self.rwlock.try_write_for(Duration::from_secs(1)) {
Some(guard) => Ok(CRWGuard::Write(guard)),
None => Err(LockError::Timeout),
}
}
}
#[derive(Debug)]
pub enum LockError {
Timeout,
}| unsafe extern "C" fn Graph_AcquireReadLock(g: *mut Graph) -> CRWGuard { | ||
| (&mut *g).acquire_read_lock() | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_AcquireWriteLock(g: *mut Graph) -> CRWGuard { | ||
| (&mut *g).acquire_write_lock() | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_ReleaseLock( | ||
| g: *mut Graph, | ||
| guard: CRWGuard, | ||
| ) { | ||
| drop(guard); | ||
| } |
There was a problem hiding this comment.
Add null pointer checks and error handling in lock operations.
The lock management functions lack null pointer checks and proper error handling, which could lead to undefined behavior.
Apply these improvements:
unsafe extern "C" fn Graph_AcquireReadLock(g: *mut Graph) -> Option<CRWGuard> {
if g.is_null() {
return None;
}
match (&mut *g).acquire_read_lock() {
Ok(guard) => Some(guard),
Err(_) => None,
}
}
unsafe extern "C" fn Graph_AcquireWriteLock(g: *mut Graph) -> Option<CRWGuard> {
if g.is_null() {
return None;
}
match (&mut *g).acquire_write_lock() {
Ok(guard) => Some(guard),
Err(_) => None,
}
}
unsafe extern "C" fn Graph_ReleaseLock(
g: *mut Graph,
guard: CRWGuard,
) -> bool {
if g.is_null() {
return false;
}
drop(guard);
true
}| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_New( | ||
| node_cap: u64, | ||
| edge_cap: u64, | ||
| ) -> *mut Graph { | ||
| Box::into_raw(Box::new(Graph::new(node_cap, edge_cap))) | ||
| } |
There was a problem hiding this comment.
Add allocation failure handling in Graph_New.
The function doesn't handle potential allocation failures, which could lead to undefined behavior.
Apply this improvement:
unsafe extern "C" fn Graph_New(
node_cap: u64,
edge_cap: u64,
) -> *mut Graph {
match Box::try_new(Graph::new(node_cap, edge_cap)) {
Ok(graph) => Box::into_raw(graph),
Err(_) => std::ptr::null_mut(),
}
}| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_PartialFree(g: *mut Graph) { | ||
| (&mut *g).set_partial(); | ||
| drop(Box::from_raw(g)); | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_Free(g: *mut Graph) { | ||
| drop(Box::from_raw(g)); | ||
| } |
There was a problem hiding this comment.
Add double-free prevention in Graph_Free and Graph_PartialFree.
These functions could lead to double-free issues if called multiple times on the same pointer.
Apply this improvement:
use std::sync::atomic::{AtomicPtr, Ordering};
static GRAPH_PTR: AtomicPtr<Graph> = AtomicPtr::new(std::ptr::null_mut());
unsafe extern "C" fn Graph_Free(g: *mut Graph) -> bool {
if g.is_null() {
return false;
}
if GRAPH_PTR.compare_exchange(
g,
std::ptr::null_mut(),
Ordering::SeqCst,
Ordering::SeqCst,
).is_ok() {
drop(Box::from_raw(g));
true
} else {
false
}
}| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_CreateNode( | ||
| g: *mut Graph, | ||
| n: *mut Node, | ||
| labels: *mut LabelID, | ||
| label_count: u32, | ||
| ) { | ||
| (&mut *g).create_node( | ||
| n.as_mut().unwrap(), | ||
| if label_count == 0 { | ||
| &[] | ||
| } else { | ||
| from_raw_parts(labels, label_count as usize) | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_CreateEdge( | ||
| g: *mut Graph, | ||
| src: NodeID, | ||
| dest: NodeID, | ||
| r: RelationID, | ||
| e: *mut Edge, | ||
| ) { | ||
| (&mut *g).create_edge(src, dest, r, e.as_mut().unwrap()); | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| #[allow(non_snake_case)] | ||
| unsafe extern "C" fn Graph_CreateEdges( | ||
| g: *mut Graph, | ||
| r: RelationID, | ||
| edges: *mut *mut Edge, | ||
| count: u32, | ||
| ) { | ||
| let edges = from_raw_parts_mut(edges, count as usize); | ||
| (&mut *g).create_edges(r, edges); | ||
| } |
There was a problem hiding this comment.
Improve safety in node and edge creation functions.
The functions lack null pointer checks and proper error handling.
Apply these improvements:
unsafe extern "C" fn Graph_CreateNode(
g: *mut Graph,
n: *mut Node,
labels: *mut LabelID,
label_count: u32,
) -> bool {
if g.is_null() || n.is_null() || (label_count > 0 && labels.is_null()) {
return false;
}
let label_slice = if label_count == 0 {
&[]
} else {
match usize::try_from(label_count) {
Ok(count) => from_raw_parts(labels, count),
Err(_) => return false,
}
};
match (&mut *g).create_node(&mut *n, label_slice) {
Ok(_) => true,
Err(_) => false,
}
}
unsafe extern "C" fn Graph_CreateEdge(
g: *mut Graph,
src: NodeID,
dest: NodeID,
r: RelationID,
e: *mut Edge,
) -> bool {
if g.is_null() || e.is_null() {
return false;
}
match (&mut *g).create_edge(src, dest, r, &mut *e) {
Ok(_) => true,
Err(_) => false,
}
}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/graph/matrix/tensor.rs (2)
352-371:⚠️ Potential issueAdd error handling in row_degree method.
The method assumes GraphBLAS operations always succeed and doesn't handle potential errors.
pub fn row_degree(&self, id: u64) -> u64 { let mut degree = 0; let mut it = DeltaMatrixIter::new_range(&self.m, id, id, false); while let Ok(Some((_, _, id))) = it.next_u64() { if single_edge(id) { degree += 1; } else { let v = clear_msb(id) as GrB_Vector; let mut nvals = MaybeUninit::uninit(); unsafe { - GrB_Vector_nvals(nvals.as_mut_ptr(), v); - degree += nvals.assume_init(); + match GrB_Vector_nvals(nvals.as_mut_ptr(), v) { + GrB_Info::GrB_SUCCESS => degree += nvals.assume_init(), + err => { + log::error!("Failed to get vector values: {:?}", err); + return 0; + } + } } } } degree }
533-533:⚠️ Potential issueReplace debug assertions with proper error handling.
Debug assertions are used to check GraphBLAS operation results. These should be replaced with proper error handling as they will be stripped in release builds.
- debug_assert!(info == GrB_Info::GrB_SUCCESS); + if info != GrB_Info::GrB_SUCCESS { + log::error!("GraphBLAS iterator operation failed: {:?}", info); + return None; + }
🧹 Nitpick comments (3)
src/graph/matrix/tensor.rs (3)
27-37: Add documentation for bit manipulation functions.These utility functions are crucial for edge management but lack documentation explaining their purpose and behavior. Consider adding doc comments to explain:
- What constitutes a "single edge"
- The significance of the MSB in edge representation
- When and why these functions should be used
+/// Returns true if the edge ID represents a single edge (MSB is not set). pub fn single_edge(current_edge: EdgeID) -> bool { current_edge as u64 & (1u64 << (u64::BITS as usize - 1)) == 0 } +/// Sets the most significant bit of the edge ID to mark it as a vector of edges. pub fn set_msb(meid: EdgeID) -> u64 { meid as u64 | (1u64 << (u64::BITS as usize - 1)) } +/// Clears the most significant bit of the edge ID to get the underlying vector pointer. pub fn clear_msb(meid: EdgeID) -> u64 { meid as u64 & !(1u64 << (u64::BITS as usize - 1)) }
39-78: Document the Tensor struct and its cleanup behavior.The Tensor struct and its Drop implementation handle complex resource management but lack documentation explaining:
- The purpose and lifecycle of the Tensor
- Why UNARYOP is needed and how it's used
- The cleanup process in the Drop implementation
+/// A tensor structure that manages a graph's edge data using a DeltaMatrix. +/// It supports both single edges and vectors of edges, using the MSB of edge IDs +/// to distinguish between them. pub struct Tensor { pub m: DeltaMatrix, } static INIT: Once = Once::new(); static mut UNARYOP: GrB_UnaryOp = null_mut(); +/// Callback function used during cleanup to free any vectors stored in the matrix. +/// This is called for each non-zero element in the matrix during Drop. #[no_mangle] #[allow(non_snake_case)] unsafe extern "C" fn _free_vectors(
1-565: Consider adding comprehensive documentation and error handling strategy.The codebase would benefit from:
- Module-level documentation explaining the tensor implementation strategy
- Error type definitions for GraphBLAS operations
- Consistent error handling patterns across all methods
- Documentation of safety requirements for unsafe blocks
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/graph/matrix/tensor.rs(1 hunks)
🔇 Additional comments (2)
src/graph/matrix/tensor.rs (2)
90-115:
Fix memory and error handling issues in set_element.
The method has several issues:
- No error handling for GraphBLAS operations
- Memory leak in the vector allocation path
- Unsafe block is too large
pub fn set_element(&mut self, src: u64, dest: u64, id: u64) {
if let Some(current_edge) = self.m.extract_element_u64(src, dest) {
unsafe {
if single_edge(current_edge) {
let mut v = MaybeUninit::uninit();
- GrB_Vector_new(v.as_mut_ptr(), GrB_BOOL, GrB_INDEX_MAX);
- let v = v.assume_init();
+ let info = GrB_Vector_new(v.as_mut_ptr(), GrB_BOOL, GrB_INDEX_MAX);
+ if info != GrB_Info::GrB_SUCCESS {
+ return;
+ }
+ let mut v = v.assume_init();
self.m.set_element_u64(set_msb(v as u64), src, dest);
- GrB_Vector_setElement_BOOL(v, true, current_edge);
- GrB_Vector_setElement_BOOL(v, true, id);
- GrB_Vector_wait(v, GrB_WaitMode::GrB_MATERIALIZE);
+ if let Err(_) = unsafe {
+ GrB_Vector_setElement_BOOL(v, true, current_edge);
+ GrB_Vector_setElement_BOOL(v, true, id);
+ GrB_Vector_wait(v, GrB_WaitMode::GrB_MATERIALIZE)
+ } {
+ GrB_Vector_free(&mut v);
+ return;
+ }
} else {
let v = clear_msb(current_edge) as GrB_Vector;
GrB_Vector_setElement_BOOL(v, true, id);
GrB_Vector_wait(v, GrB_WaitMode::GrB_MATERIALIZE);
}
}
} else {
self.m.set_element_u64(id, src, dest);
}
}Likely invalid or redundant comment.
466-497:
Improve safety in TensorIterator implementation.
The iterator implementation has several safety concerns:
- Unchecked GraphBLAS operations
- Potential null pointer dereference
- No cleanup of resources on error
fn next(&mut self) -> Option<Self::Item> {
if self.deleted {
return None;
}
if single_edge(self.edge_id) {
self.deleted = true;
Some((self.src_id, self.dest_id, self.edge_id))
} else {
if self.eit.is_none() {
let v = clear_msb(self.edge_id) as GrB_Vector;
let mut it = MaybeUninit::uninit();
unsafe {
- GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut());
- GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0)
+ if GxB_Vector_Iterator_attach(it.as_mut_ptr(), v, null_mut()) != GrB_Info::GrB_SUCCESS {
+ self.deleted = true;
+ return None;
+ }
+ if GxB_Vector_Iterator_seek(it.as_mut_ptr(), 0) != GrB_Info::GrB_SUCCESS {
+ self.deleted = true;
+ return None;
+ }
};
self.eit = Some(unsafe { it.assume_init() });
}
unsafe {
+ let Some(it) = self.eit.as_mut() else {
+ return None;
+ };
- let edge_id = GxB_Vector_Iterator_getIndex(self.eit.as_mut().unwrap());
- let info = GxB_Vector_Iterator_next(self.eit.as_mut().unwrap());
+ let edge_id = GxB_Vector_Iterator_getIndex(it);
+ let info = GxB_Vector_Iterator_next(it);
if info == GrB_Info::GxB_EXHAUSTED {
self.eit = None;
self.deleted = true;
}
return Some((self.src_id, self.dest_id, edge_id));
}
}
}Likely invalid or redundant comment.
| @@ -17,6 +23,7 @@ fn main() { | |||
| println!("cargo:rustc-link-arg=-L/opt/homebrew/opt/libomp/lib"); | |||
There was a problem hiding this comment.
Why do we have duplication of the link flags ?
Summary by CodeRabbit
New Features
CRWLockfor improved concurrency control.DataBlockandDataBlockIteratortypes for efficient data management.Bug Fixes
Documentation
Chores
parking_lotcrate for better mutex management.