Don't measure in memory geo, map and numeric indices#6311
Conversation
a237b6d to
281fe19
Compare
6c158a8 to
2758672
Compare
281fe19 to
d179174
Compare
2758672 to
f85d509
Compare
📝 WalkthroughWalkthroughThe changes remove the Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (12)
🧰 Additional context used🧬 Code Definitions (1)lib/common/common/src/counter/iterator_hw_measurement.rs (2)
⏰ Context from checks skipped due to timeout of 90000ms (13)
🔇 Additional comments (2)
✨ Finishing Touches
🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/segment/src/index/field_index/map_index/mutable_map_index.rs (1)
128-144: Simplified method signature by removing hardware counter parameterThe
check_values_anymethod now has a simpler signature with the hardware counter parameter removed. The hardware counter tracking logic (hw_counter_val += N::mmapped_size(item.as_referenced())) remains in the code but is no longer used anywhere, which creates dead code.Consider removing the unused hardware counter tracking logic since it's no longer serving any purpose:
pub fn check_values_any(&self, idx: PointOffsetType, check_fn: impl Fn(&N) -> bool) -> bool { - let mut hw_counter_val = 0; - let res = self .point_to_values .get(idx as usize) .map(|values| { values.iter().any(|v| { let item: &N = v.borrow(); - hw_counter_val += N::mmapped_size(item.as_referenced()); check_fn(item) }) }) .unwrap_or(false); res }lib/segment/src/index/field_index/map_index/immutable_map_index.rs (1)
251-261: Simplified method signature by removing hardware counter parameterThe
check_values_anymethod now has a simpler signature with the hardware counter parameter removed. Similar to the mutable version, the hardware counter tracking logic (hw_count_val += <N as MmapValue>::mmapped_size(v.as_referenced())) remains in the code but is no longer used anywhere.Consider removing the unused hardware counter tracking logic since it's no longer serving any purpose:
pub fn check_values_any(&self, idx: PointOffsetType, check_fn: impl Fn(&N) -> bool) -> bool { - let mut hw_count_val = 0; - let res = self.point_to_values.check_values_any(idx, |v| { let v = v.borrow(); - hw_count_val += <N as MmapValue>::mmapped_size(v.as_referenced()); check_fn(v) }); res }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
lib/segment/src/index/field_index/geo_index/immutable_geo_index.rs(2 hunks)lib/segment/src/index/field_index/geo_index/mmap_geo_index.rs(8 hunks)lib/segment/src/index/field_index/geo_index/mod.rs(2 hunks)lib/segment/src/index/field_index/geo_index/mutable_geo_index.rs(3 hunks)lib/segment/src/index/field_index/map_index/immutable_map_index.rs(4 hunks)lib/segment/src/index/field_index/map_index/mmap_map_index.rs(9 hunks)lib/segment/src/index/field_index/map_index/mod.rs(3 hunks)lib/segment/src/index/field_index/map_index/mutable_map_index.rs(2 hunks)lib/segment/src/index/field_index/numeric_index/immutable_numeric_index.rs(2 hunks)lib/segment/src/index/field_index/numeric_index/mmap_numeric_index.rs(6 hunks)lib/segment/src/index/field_index/numeric_index/mod.rs(3 hunks)lib/segment/src/index/field_index/numeric_index/mutable_numeric_index.rs(4 hunks)
🧰 Additional context used
🧬 Code Definitions (9)
lib/segment/src/index/field_index/numeric_index/mmap_numeric_index.rs (4)
lib/segment/src/types.rs (9)
is_on_disk(1180-1182)is_on_disk(1317-1322)is_on_disk(1736-1747)is_on_disk(1789-1794)new(376-389)new(1463-1466)new(2601-2603)new(2825-2827)new(2838-2840)lib/segment/src/index/field_index/map_index/mmap_map_index.rs (1)
make_conditioned_counter(329-334)lib/segment/src/index/field_index/numeric_index/mod.rs (1)
new(436-441)lib/segment/src/index/field_index/map_index/mutable_map_index.rs (1)
new(26-39)
lib/segment/src/index/field_index/geo_index/mod.rs (3)
lib/segment/src/index/field_index/geo_index/mmap_geo_index.rs (1)
values_of_hash(305-320)lib/segment/src/index/field_index/geo_index/immutable_geo_index.rs (1)
values_of_hash(110-116)lib/segment/src/index/field_index/geo_index/mutable_geo_index.rs (1)
values_of_hash(242-244)
lib/segment/src/index/field_index/geo_index/mmap_geo_index.rs (6)
lib/segment/src/types.rs (9)
is_on_disk(1180-1182)is_on_disk(1317-1322)is_on_disk(1736-1747)is_on_disk(1789-1794)new(376-389)new(1463-1466)new(2601-2603)new(2825-2827)new(2838-2840)lib/segment/src/index/field_index/map_index/mmap_map_index.rs (1)
make_conditioned_counter(329-334)lib/segment/src/index/field_index/numeric_index/mmap_numeric_index.rs (1)
make_conditioned_counter(353-358)lib/segment/src/index/field_index/bool_index/mmap_bool_index.rs (1)
make_conditioned_counter(88-94)lib/segment/src/index/field_index/geo_index/immutable_geo_index.rs (1)
new(37-51)lib/segment/src/index/field_index/geo_index/mutable_geo_index.rs (2)
new(54-63)new(202-212)
lib/segment/src/index/field_index/map_index/mmap_map_index.rs (4)
lib/segment/src/types.rs (5)
is_on_disk(1180-1182)is_on_disk(1317-1322)is_on_disk(1736-1747)is_on_disk(1789-1794)i(3035-3035)lib/segment/src/index/field_index/geo_index/mmap_geo_index.rs (2)
make_conditioned_counter(394-399)new(86-195)lib/segment/src/index/field_index/numeric_index/mmap_numeric_index.rs (1)
make_conditioned_counter(353-358)lib/segment/src/index/field_index/bool_index/mmap_bool_index.rs (1)
make_conditioned_counter(88-94)
lib/segment/src/index/field_index/geo_index/immutable_geo_index.rs (2)
lib/segment/src/index/field_index/geo_index/mod.rs (2)
points_of_hash(112-118)values_of_hash(120-126)lib/segment/src/index/field_index/geo_index/mutable_geo_index.rs (2)
points_of_hash(238-240)values_of_hash(242-244)
lib/segment/src/index/field_index/numeric_index/mutable_numeric_index.rs (10)
lib/segment/src/index/field_index/map_index/immutable_map_index.rs (1)
check_values_any(251-261)lib/segment/src/index/field_index/map_index/mutable_map_index.rs (1)
check_values_any(128-144)lib/segment/src/index/field_index/geo_index/mod.rs (1)
check_values_any(185-196)lib/segment/src/index/field_index/numeric_index/mod.rs (1)
check_values_any(245-256)lib/segment/src/index/field_index/geo_index/immutable_geo_index.rs (1)
check_values_any(73-84)lib/segment/src/index/field_index/geo_index/mmap_geo_index.rs (1)
check_values_any(247-268)lib/segment/src/index/field_index/map_index/mmap_map_index.rs (1)
check_values_any(164-187)lib/segment/src/index/field_index/numeric_index/immutable_numeric_index.rs (3)
check_values_any(169-175)values_range(68-80)values_range(202-210)lib/segment/src/index/field_index/numeric_index/mmap_numeric_index.rs (2)
check_values_any(209-226)values_range(254-269)lib/segment/src/index/field_index/bool_index/mod.rs (1)
check_values_any(66-76)
lib/segment/src/index/field_index/geo_index/mutable_geo_index.rs (1)
lib/segment/src/index/field_index/geo_index/immutable_geo_index.rs (5)
check_values_any(73-84)values_count(90-94)points_per_hash(96-100)points_of_hash(102-108)values_of_hash(110-116)
lib/segment/src/index/field_index/map_index/mutable_map_index.rs (8)
lib/segment/src/index/field_index/map_index/immutable_map_index.rs (1)
check_values_any(251-261)lib/segment/src/index/field_index/numeric_index/mutable_numeric_index.rs (2)
check_values_any(73-78)check_values_any(284-286)lib/segment/src/index/field_index/geo_index/mod.rs (1)
check_values_any(185-196)lib/segment/src/index/field_index/map_index/mod.rs (1)
check_values_any(122-133)lib/segment/src/index/field_index/numeric_index/mod.rs (1)
check_values_any(245-256)lib/segment/src/index/field_index/map_index/mmap_map_index.rs (1)
check_values_any(164-187)lib/segment/src/index/field_index/numeric_index/immutable_numeric_index.rs (1)
check_values_any(169-175)lib/segment/src/index/field_index/geo_index/mutable_geo_index.rs (1)
check_values_any(214-223)
lib/segment/src/index/field_index/map_index/immutable_map_index.rs (7)
lib/segment/src/index/field_index/map_index/mutable_map_index.rs (1)
check_values_any(128-144)lib/segment/src/index/field_index/geo_index/mod.rs (1)
check_values_any(185-196)lib/segment/src/index/field_index/map_index/mod.rs (1)
check_values_any(122-133)lib/segment/src/index/field_index/geo_index/immutable_geo_index.rs (1)
check_values_any(73-84)lib/segment/src/index/field_index/map_index/mmap_map_index.rs (1)
check_values_any(164-187)lib/segment/src/index/field_index/numeric_index/immutable_numeric_index.rs (1)
check_values_any(169-175)lib/segment/src/index/field_index/numeric_index/mmap_numeric_index.rs (1)
check_values_any(209-226)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: test-snapshot-operations-s3-minio
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-low-resources
- GitHub Check: test-consistency
- GitHub Check: Basic TLS/HTTPS tests
- GitHub Check: test (macos-latest)
- GitHub Check: test-consensus-compose
- GitHub Check: test (windows-latest)
- GitHub Check: test-consensus
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (41)
lib/segment/src/index/field_index/numeric_index/mod.rs (3)
252-254: Hardware counter handling simplified for in-memory indexesThe code now differentiates hardware counter behavior between in-memory indexes (Mutable and Immutable) and disk-based index (Mmap). In-memory indexes no longer receive the hardware counter parameter, simplifying their API and improving maintainability.
376-378: Consistent hardware counter removal patternThe removal of the hardware counter parameter from in-memory index variants is consistently applied here in the
point_ids_by_valuemethod, aligning with the changes in thecheck_values_anymethod above.
701-710: Hardware counter handling streamlined for filter methodThe hardware counter parameter has been removed from the values_range call for in-memory indexes while being maintained for mmap indexes, consistent with the approach throughout this PR. This creates a cleaner separation between hardware measurement logic for in-memory vs. on-disk indexes.
lib/segment/src/index/field_index/numeric_index/mmap_numeric_index.rs (4)
36-36: Added on-disk tracking capabilityThe addition of the
is_on_diskboolean field enables the index to track whether it's using disk-based storage, which will influence hardware counter behavior.
180-181: Properly initializing the on-disk stateThe
is_on_diskfield is correctly initialized with the parameter value passed to theloadmethod, ensuring the index can properly track its storage location.
215-216: Conditional hardware counter for performance measurementThe code now uses a conditioned hardware counter based on the disk status of the index, which allows for more accurate performance measurements. This approach ensures the hardware counter only tracks operations that are truly I/O-bound.
Also applies to: 260-261
353-358: Helper method for creating conditioned countersThe new
make_conditioned_countermethod encapsulates the logic for creating a counter that behaves differently based on the disk status. This improves code organization and ensures consistent counter behavior across methods.lib/segment/src/index/field_index/map_index/mmap_map_index.rs (4)
38-38: Added on-disk tracking for map indexSimilar to the numeric index, the map index now tracks whether it's stored on disk, enabling more accurate hardware counter behavior.
167-171: Improved parameter naming and conditioned counter usageThe parameter has been renamed from
hw_acctohw_counterfor consistency across the codebase, and the method now creates a conditioned counter based on the disk status.
319-324: Updated measurement methodology for iteratorsThe code now uses
measure_hw_with_accinstead ofmeasure_hw_with_cell, passing the conditioned counter's accumulator. This ensures consistent measurement behavior across the codebase.
329-334: Consistent helper method for counter conditioningThe implementation of
make_conditioned_counteris consistent with other index types, reinforcing the systematic approach to hardware counter management across the codebase.lib/segment/src/index/field_index/geo_index/mmap_geo_index.rs (3)
76-76: Added on-disk tracking capability for geo indexThe geo index now includes an
is_on_diskfield, consistent with the pattern established in other index implementations.
253-254: Consistent conditioning of hardware countersThe geo index implementation applies the conditioned counter pattern consistently across all methods that perform hardware counter measurement, ensuring uniform behavior.
Also applies to: 289-290, 306-307
394-399: Standardized helper method implementationThe
make_conditioned_counterimplementation is identical to those in other index types, ensuring consistent behavior across different index implementations.lib/segment/src/index/field_index/map_index/mod.rs (5)
129-130: HardwareCounterCell parameter removed for in-memory indicesThe
hw_counterparameter has been removed from thecheck_values_anymethod calls for bothMutableandImmutablevariants, while it is still being passed to theMmapvariant. This change aligns with the PR objective to stop measuring in-memory operations.
184-185: Consistent hardware counter removal from in-memory indicesThe
hw_counterparameter has been removed fromget_count_for_valuemethod calls for both memory-based variants, consistent with the pattern applied to other methods in this PR.
192-193: Hardware counter parameter removed for in-memory iterator retrievalThe
hw_counterparameter has been consistently removed fromget_iteratormethod calls forMutableandImmutablevariants while maintaining it for theMmapvariant, which likely involves disk I/O operations.
219-220: Removed hardware counter for in-memory values map iterationThe
hw_counterparameter has been removed fromiter_values_mapmethod calls for memory-based variants, completing the consistent pattern of removing hardware counter measurements from in-memory operations throughout the file.
122-132:Details
✅ Verification successful
Verify API compatibility for hardware counter removals
While the changes are consistent within this file, it's important to ensure that the underlying implementations in
MutableMapIndexandImmutableMapIndexclasses have been similarly updated to match these interface changes.
🏁 Script executed:
#!/bin/bash # Check if the method signatures in the underlying implementations have been updated echo "Checking MutableMapIndex implementation:" rg -A 2 "fn check_values_any" lib/segment/src/index/field_index/map_index/mutable_map_index.rs echo -e "\nChecking ImmutableMapIndex implementation:" rg -A 2 "fn check_values_any" lib/segment/src/index/field_index/map_index/immutable_map_index.rs echo -e "\nChecking MmapMapIndex implementation (should still use hw_counter):" rg -A 2 "fn check_values_any" lib/segment/src/index/field_index/map_index/mmap_map_index.rsLength of output: 974
API Compatibility Verified for Hardware Counter Adjustments
The executed script confirms that the
check_values_anymethod in bothMutableMapIndexandImmutableMapIndexhas been updated to remove thehw_counterparameter, aligning with the changes inmod.rs. TheMmapMapIndeximplementation still expects thehw_counterparameter, as intended.
- MutableMapIndex: Method signature now only accepts
idxandcheck_fn.- ImmutableMapIndex: Method signature updated similarly.
- MmapMapIndex: Continues to use
hw_counter, consistent with its intended behavior.No further modifications are required at this time.
lib/segment/src/index/field_index/geo_index/mod.rs (3)
112-117: Removed hardware counter parameter from in-memory indices.The hardware counter parameter has been appropriately removed from the
points_of_hashmethod for both mutable and immutable indices, while maintaining it for memory-mapped indices. This is consistent with the PR objective of not measuring in-memory geo indices.
120-125: Removed hardware counter parameter from in-memory indices.The hardware counter parameter has been successfully removed from the
values_of_hashmethod for both mutable and immutable indices, while maintaining it for memory-mapped indices. This change follows the same pattern as thepoints_of_hashmethod.
185-196: Removed hardware counter parameter from in-memory indices.The hardware counter parameter has been appropriately removed from the
check_values_anymethod for mutable and immutable variants, while correctly maintaining it for memory-mapped indices. This change completes the consistent pattern of removing hardware counter tracking from in-memory index operations.lib/segment/src/index/field_index/numeric_index/immutable_numeric_index.rs (2)
169-175: Simplified method implementation by removing hardware counter.The
check_values_anymethod has been streamlined by removing the hardware counter tracking logic, directly delegating to the underlyingpoint_to_valuesimplementation. This simplification aligns with the PR objective.
202-210: Removed hardware counter parameter from method signature.The
values_rangemethod no longer accepts a hardware counter parameter, and the implementation has been simplified to focus solely on the core functionality of returning mapped indices. This change is consistent with the overall PR objective.lib/segment/src/index/field_index/geo_index/immutable_geo_index.rs (3)
73-84: Simplified check_values_any implementation.The method has been simplified by removing the hardware counter tracking logic. Now it directly returns the result from
point_to_values.check_values_anywithout incrementing a counter. This simplification aligns with the PR objective and makes the code more readable.
102-108: Removed hardware counter parameter from method signature.The
points_of_hashmethod now has a simpler signature without the hardware counter parameter. The implementation remains focused on the core functionality of retrieving point counts for a given hash.
110-116: Removed hardware counter parameter from method signature.The
values_of_hashmethod has been updated to remove the hardware counter parameter, consistent with the other changes in the PR. The implementation continues to correctly retrieve value counts for a given hash.lib/segment/src/index/field_index/geo_index/mutable_geo_index.rs (4)
182-187: Updated delegate macro to reflect simpler method signatures.The
delegate!macro has been updated to match the new method signatures that don't include hardware counter parameters forcheck_values_any,points_of_hash, andvalues_of_hash. This ensures consistency between the MutableGeoMapIndex and InMemoryGeoMapIndex implementations.
214-223: Simplified check_values_any implementation.The implementation has been streamlined by directly returning the result of
values.iter().any(check_fn)without extra hardware counter tracking logic. This makes the code cleaner and more focused on its core functionality.
238-240: Removed hardware counter parameter from method signature.The
points_of_hashmethod now has a simpler interface without the hardware counter parameter. This is consistent with the overall PR goal of not measuring in-memory indices.
242-244: Removed hardware counter parameter from method signature.The
values_of_hashmethod has been simplified by removing the hardware counter parameter, consistent with the other changes in the PR. The implementation remains focused on retrieving value counts from the hash map.lib/segment/src/index/field_index/map_index/mutable_map_index.rs (3)
171-173: Simplified method signature by removing hardware counter parameterThe
get_count_for_valuemethod has been simplified by removing the hardware counter parameter and the associated counter incrementation logic.
179-183: Simplified method signature by removing hardware counter parameterThe
iter_values_mapmethod signature has been simplified by removing the hardware counter parameter. The implementation is now more straightforward without the need to track hardware access.
185-189: Simplified method signature by removing hardware counter parameterThe
get_iteratormethod signature has been simplified by removing the hardware counter parameter. The implementation no longer tracks hardware access for iterating over values.lib/segment/src/index/field_index/map_index/immutable_map_index.rs (3)
283-287: Simplified method signature by removing hardware counter parameterThe
get_count_for_valuemethod has been simplified by removing the hardware counter parameter and associated counter incrementation logic.
295-302: Simplified method signature by removing hardware counter parameterThe
iter_values_mapmethod signature has been simplified by removing the hardware counter parameter. Line 299 no longer needs to track hardware counter increments.
304-325: Simplified method signature by removing hardware counter parameterThe
get_iteratormethod has been simplified by removing the hardware counter parameter. The implementation no longer tracks hardware access for range operations.lib/segment/src/index/field_index/numeric_index/mutable_numeric_index.rs (4)
73-78: Simplified method signature by removing hardware counter parameterThe
check_values_anymethod in theInMemoryNumericIndexstruct has been simplified by removing the hardware counter parameter. The implementation is now more straightforward with a focus solely on checking if any values match the condition.
96-104: Simplified method signature by removing hardware counter parameterThe
values_rangemethod has been simplified by removing the hardware counter parameter. The implementation now directly returns an iterator over the range without tracking hardware access.
284-286: Simplified method signature by removing hardware counter parameterThe
check_values_anymethod in theMutableNumericIndexstruct has been simplified by removing the hardware counter parameter. It now forwards the call directly to the corresponding method inInMemoryNumericIndex.
300-306: Simplified method signature by removing hardware counter parameterThe
values_rangemethod in theMutableNumericIndexstruct has been simplified by removing the hardware counter parameter. It now directly delegates to theInMemoryNumericIndeximplementation.
f85d509 to
3bd4477
Compare
3bd4477 to
edc0118
Compare
Depends on #6309