Optimize has_id with ahash #6804
Conversation
📝 WalkthroughWalkthroughThis change updates the Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Clippy (1.86.0)error: failed to load source for dependency Caused by: Caused by: Caused by: 📜 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 (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/segment/src/index/query_optimization/condition_converter.rs (1)
186-186: Consider consistency: Why not optimize CustomIdChecker too?The
CustomIdCheckercondition also uses HashSet for storing internal IDs and performing membership checks, similar to thehas_idcondition. For consistency and potential performance gains, consider using AHashSet here as well.- let segment_ids: HashSet<_> = id_tracker + let segment_ids: AHashSet<_> = id_trackerIf there's a specific reason to keep HashSet for CustomIdChecker (e.g., different usage patterns or performance characteristics), please clarify in the code comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/segment/src/index/query_optimization/condition_converter.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JojiiOfficial
PR: qdrant/qdrant#5951
File: lib/api/src/grpc/qdrant.rs:6493-6496
Timestamp: 2025-03-12T09:25:16.340Z
Learning: The `payload_index_io_read` field in `HardwareUsage` was introduced in PR #5951 and is safe to modify as it was not used in production before this PR.
Learnt from: coszio
PR: qdrant/qdrant#6565
File: lib/posting_list/src/builder.rs:63-67
Timestamp: 2025-05-26T14:47:23.505Z
Learning: In the posting_list crate's PostingChunk struct, avoid adding extra fields like storing computed chunk_bits to prevent struct bloat and maintain mmap compatibility. The bit calculations from offsets are inexpensive compared to the memory and compatibility benefits of keeping the struct minimal.
lib/segment/src/index/query_optimization/condition_converter.rs (1)
Learnt from: IvanPleshkov
PR: qdrant/qdrant#6728
File: lib/quantization/src/encoded_vectors_binary.rs:810-810
Timestamp: 2025-07-02T17:10:13.847Z
Learning: In the Qdrant quantization codebase, hardware counters (hw_counter.cpu_counter()) are used to measure vector data access from storage, not computational cost. With asymmetric binary quantization where query vectors can be longer than storage vectors, the counter should still track vector_data.len() to maintain consistent measurement of storage access patterns, not query processing overhead.
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Basic TLS/HTTPS tests
- GitHub Check: test-low-resources
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-consistency
- GitHub Check: test-consensus-compose
- GitHub Check: test-snapshot-operations-s3-minio
- GitHub Check: lint
- GitHub Check: integration-tests-consensus
- GitHub Check: integration-tests
- GitHub Check: storage-compat-test
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: rust-tests (ubuntu-latest)
🔇 Additional comments (2)
lib/segment/src/index/query_optimization/condition_converter.rs (2)
3-3: Good optimization choice for performance-critical path.Adding AHashSet import to optimize membership checking operations. AHash is generally faster than the default hasher for this use case.
113-113: Excellent optimization for has_id membership checking.Replacing HashSet with AHashSet for the segment_ids collection is a smart optimization. Since this is used for membership checking with u32 internal IDs, the faster AHash algorithm should provide measurable performance improvements, especially for large ID sets as mentioned in the PR description.
That is huge! Nice find. |
* Optimize has_id with ahash * apply same pattern to CustomIdChecker
This
has_idimplementation is is a prime candidate forAHashSetbecause it tests the membership in a set ofu32.With some naive benchmarking, I get a 10% latency improvements on large distance matrix requests.
e.g.