Conversation
7e68a29 to
a7815c4
Compare
1095229 to
3a256e9
Compare
a7815c4 to
64574c3
Compare
lib/collection/src/collection/mmr.rs
Outdated
| if !upper_offsets.is_empty() { | ||
| let mut scores = vec![0.0; upper_offsets.len()]; | ||
| raw_scorers[i].score_points(&upper_offsets, &mut scores); | ||
| scores.resize(upper_offsets.len(), 0.0); |
There was a problem hiding this comment.
Also added a minor optimization here to reduce allocations and share scores across loops.
Note: we don't need to clear it because we always overwrite all items before using them.
5e26ae9 to
6aa9f13
Compare
📝 WalkthroughWalkthroughThe changes introduce a new generic 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/collection/src/common/symmetric_matrix.rs (1)
53-72: Mathematically correct index calculation.The index calculation logic is sound:
- Uses the formula
((row * size) + column) - triangular(row)to map 2D coordinates to the flattened upper triangle- Triangular number calculation
(x * (x + 1)) / 2is correct- Debug assertion helps catch column out-of-bounds during development
Consider adding a debug assertion for row bounds as well for consistency:
debug_assert!( column < self.size, "Column index {column} out of bounds for {}x{} matrix", self.size, self.size ); +debug_assert!( + row < self.size, + "Row index {row} out of bounds for {}x{} matrix", + self.size, + self.size +);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/collection/src/collection/mmr.rs(6 hunks)lib/collection/src/common/mod.rs(1 hunks)lib/collection/src/common/symmetric_matrix.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: lint
- GitHub Check: integration-tests
- GitHub Check: test-snapshot-operations-s3-minio
- GitHub Check: Basic TLS/HTTPS tests
- GitHub Check: test-low-resources
- GitHub Check: test-consensus-compose
- GitHub Check: integration-tests-consensus
- GitHub Check: test-consistency
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: storage-compat-test
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
🔇 Additional comments (11)
lib/collection/src/common/mod.rs (1)
14-14: Clean module export addition.The new symmetric_matrix module export follows the existing pattern and correctly makes the SymmetricMatrix struct publicly available for use in the MMR implementation.
lib/collection/src/common/symmetric_matrix.rs (4)
1-24: Well-designed constructor with proper validation.The struct definition and constructor are well-implemented:
- Generic design allows flexibility for different numeric types
- Proper validation prevents matrices smaller than 2x2 (which wouldn't benefit from the optimization)
- Clear documentation about the storage approach
- Correct size calculation using triangular numbers
26-41: Efficient and correct access methods.The get and set methods properly handle the symmetric matrix abstraction:
- Both methods correctly delegate to
handle_indexfor symmetry handling- Inline annotations are appropriate for performance-critical code
- The API is intuitive and maintains the illusion of a full 2D matrix
43-51: Correct symmetry handling.The
handle_indexmethod correctly ensures that matrix accesses always target the upper triangle by swapping indices whencolumn < row. This is essential for maintaining the symmetric property while storing only half the data.
75-152: Comprehensive test coverage validates the implementation.The test suite is excellent:
test_matrix()verifies basic functionality and symmetrytest_oob()ensures proper bounds checkingtest_matrix_vs_naive_implementation()validates correctness against a reference implementation- Tests cover diagonal elements, off-diagonal elements, and edge cases
lib/collection/src/collection/mmr.rs (6)
18-24: Clean integration with appropriate type alias.The import and type alias definition are well-structured:
- Import follows Rust conventions
SimilarityMatrixalias makes the domain-specific usage clear- Type alias improves code readability in the MMR context
191-191: Function signature correctly updated.The return type change from
Vec<Vec<ScoreType>>toSimilarityMatrixproperly reflects the new symmetric matrix abstraction.
205-207: Proper matrix initialization with error handling.The matrix initialization correctly:
- Uses
SymmetricMatrix::new()with appropriate size and initial value- Handles the unwrap safely since the size validation is already performed above
- Maintains the same semantic behavior (zero-initialized matrix)
225-239: Excellent performance optimization with buffer reuse.The optimization to reuse the
scoresbuffer across loop iterations is well-implemented:
- Preallocates buffer once outside the loop
- Reuses different slices of the buffer for each iteration
- Reduces memory allocations while maintaining correctness
- Properly sets values using the new
setmethod
261-261: Parameter type correctly updated.The function parameter type change to
&SimilarityMatrixmaintains the same interface semantics while using the new optimized data structure.
305-305: Correct API usage for matrix access.The change from direct 2D indexing to using the
getmethod correctly adapts to the new symmetric matrix API while maintaining the same functionality.
* Add DiagonalMatrix for mmr * Add column OOB checking in debug mode * Fix Clippy; Reduce allocations in scorers loop * rename to symmetric matrix * extra test * Don't call 'resize' in each iteration * Clippy * review nits --------- Co-authored-by: Luis Cossío <luis.cossio@outlook.com>
Builds on #6797
Optimizes the diagonal matrix used in MMR to only store half of the data in a single, flattened vector.