Add iteration and standard impls in once crate#199
Add iteration and standard impls in once crate#199JoeyBF merged 15 commits intoSpectralSequences:masterfrom
once crate#199Conversation
|
For future reference, I did try unwrapping the new methods in Profiling the bad case with |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdded iteration APIs and iterator infrastructure across the once crate (Grove, TwoEndedGrove, MultiIndexed, KdTrie), exposed kdtrie publicly, implemented standard trait impls using iteration, extended benchmarks to include KdTrie and iteration workloads, and added a dev test dependency. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
ext/crates/once/Cargo.toml(1 hunks)ext/crates/once/benches/criterion/benchable_impl.rs(5 hunks)ext/crates/once/benches/criterion/main.rs(8 hunks)ext/crates/once/src/grove/mod.rs(5 hunks)ext/crates/once/src/multiindexed/iter.rs(1 hunks)ext/crates/once/src/multiindexed/kdtrie.rs(2 hunks)ext/crates/once/src/multiindexed/mod.rs(8 hunks)ext/crates/once/src/multiindexed/node.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-11T08:35:31.031Z
Learnt from: JoeyBF
Repo: SpectralSequences/sseq PR: 205
File: ext/crates/fp/src/limb.rs:11-28
Timestamp: 2025-12-11T08:35:31.031Z
Learning: In Rust 2024 edition (Cargo.toml edition = 2024), the mem::size_of, size_of_val, align_of, and align_of_val functions from std::mem are in the prelude and do not require explicit imports. During reviews, these can be used unqualified when the project targets edition = 2024. Verify the crate's Cargo.toml edition setting before assuming unqualified use, and avoid adding extraneous imports for these helpers.
Applied to files:
ext/crates/once/src/multiindexed/iter.rsext/crates/once/src/multiindexed/mod.rsext/crates/once/src/multiindexed/kdtrie.rsext/crates/once/src/grove/mod.rsext/crates/once/benches/criterion/benchable_impl.rsext/crates/once/benches/criterion/main.rsext/crates/once/src/multiindexed/node.rs
📚 Learning: 2025-12-14T08:23:53.399Z
Learnt from: JoeyBF
Repo: SpectralSequences/sseq PR: 207
File: ext/crates/fp/src/simd/x86_64/avx512.rs:367-379
Timestamp: 2025-12-14T08:23:53.399Z
Learning: In Rust 2024 edition, do not remove explicit unsafe blocks inside unsafe fn bodies. The lint unsafe_op_in_unsafe_fn is enabled by default, treating the body of an unsafe fn as safe and requiring each unsafe operation to be wrapped in an explicit unsafe { ... } block. Do not suggest removing these blocks in code using edition = "2024"; ensure unsafe operations are enclosed in explicit unsafe blocks and review for unnecessary nesting.
Applied to files:
ext/crates/once/src/multiindexed/iter.rsext/crates/once/src/multiindexed/mod.rsext/crates/once/src/multiindexed/kdtrie.rsext/crates/once/src/grove/mod.rsext/crates/once/benches/criterion/benchable_impl.rsext/crates/once/benches/criterion/main.rsext/crates/once/src/multiindexed/node.rs
🧬 Code graph analysis (5)
ext/crates/once/src/multiindexed/iter.rs (2)
ext/crates/once/src/multiindexed/kdtrie.rs (4)
new(45-55)dimensions(160-162)root(172-174)get(71-82)ext/crates/once/src/multiindexed/mod.rs (3)
new(101-103)new(395-398)get(150-152)
ext/crates/once/src/multiindexed/mod.rs (2)
ext/crates/once/src/multiindexed/kdtrie.rs (4)
iter(164-170)new(45-55)clone(190-196)get(71-82)ext/crates/once/src/multiindexed/iter.rs (5)
new(20-32)new(55-61)get(39-39)get(140-142)get(155-157)
ext/crates/once/src/multiindexed/kdtrie.rs (2)
ext/crates/once/src/multiindexed/mod.rs (3)
iter(226-228)new(101-103)new(395-398)ext/crates/once/src/multiindexed/iter.rs (2)
new(20-32)new(55-61)
ext/crates/once/benches/criterion/benchable_impl.rs (5)
ext/crates/once/src/grove/mod.rs (2)
iter(363-365)iter(636-638)ext/crates/once/src/multiindexed/kdtrie.rs (1)
iter(164-170)ext/crates/once/src/multiindexed/mod.rs (1)
iter(226-228)ext/crates/once/benches/criterion/main.rs (4)
iter(41-43)iter(63-68)iter(88-93)iter(113-119)ext/crates/once/src/once.rs (2)
iter(381-386)iter(714-716)
ext/crates/once/benches/criterion/main.rs (3)
ext/crates/once/src/multiindexed/kdtrie.rs (3)
iter(164-170)new(45-55)get(71-82)ext/crates/once/src/multiindexed/mod.rs (4)
iter(226-228)new(101-103)new(395-398)get(150-152)ext/crates/once/src/multiindexed/iter.rs (5)
new(20-32)new(55-61)get(39-39)get(140-142)get(155-157)
🔇 Additional comments (26)
ext/crates/once/Cargo.toml (1)
18-18: LGTM!Adding
expect-testas a dev-dependency for snapshot testing is appropriate. This crate is well-maintained (by the rust-analyzer team) and useful for thetest_debugtest cases.ext/crates/once/src/multiindexed/node.rs (1)
163-179: LGTM!The new
inner()andleaf()accessors are well-documented with appropriate safety requirements. They follow the existing pattern of other unsafe methods in this file and provide the necessary access for the iteration infrastructure.ext/crates/once/src/multiindexed/kdtrie.rs (2)
159-174: LGTM!The new accessor methods and
iter()implementation are well-structured. Pre-allocating theVecwithwith_capacity(self.dimensions)initer()is a good optimization to avoid reallocations during iteration.
183-197: LGTM!The
DebugandCloneimplementations are idiomatic. Usingdebug_map().entries(self.iter())provides a clear map-like representation. TheCloneimplementation correctly rebuilds the trie by iterating and inserting cloned values.ext/crates/once/src/multiindexed/iter.rs (2)
64-129: LGTM!The iterator implementation is well-structured with a clear stack-based DFS approach. The logic correctly handles:
- Leaf vs inner node distinction based on depth
- Pushing remaining ranges back onto the stack for resumption
- Coordinate tracking via the
Coordinatestrait abstractionThe comment in the docstring about the 3× speedup from using fixed-size arrays is a helpful performance note.
131-158: LGTM!The
Coordinatesimplementations for[i32; K]andVec<i32>are appropriate:
- Arrays use
Copysemantics and no-op truncation (since the array size is fixed)Vecusespush/truncatefor dynamic coordinate building with a defensive assertThe assert in
set_coordforVec<i32>(line 147) provides a good safety check that the iterator is using the trait correctly.ext/crates/once/src/multiindexed/mod.rs (5)
207-228: LGTM!The
iter()method is well-documented with a clear example. Using[0; K]as the initial coordinates array is correct since these values will be overwritten during iteration, and the array coordinates implementation doesn't require truncation.
237-274: LGTM!The standard trait implementations are correct and consistent:
- Unlike
KdTrie,MultiIndexeddoesn't need to hashKbecause it's a const generic—the type system ensures only same-K instances can be compared or hashed together.- The implementations leverage the deterministic iteration order provided by
iter().
360-378: LGTM!Good basic tests covering:
- Empty iterator behavior
- Idempotent iteration (multiple calls return same results)
434-476: LGTM!The
test_debugtest usingexpect_testfor snapshot testing is a clean approach—it makes it easy to verify the debug output format and will catch unintended changes. Thetest_clonetest appropriately verifies that cloned values are accessible and independent.
568-606: LGTM!The
proptest_multiindexed_iter_kdtest effectively validates:
- Iteration returns all inserted elements
- Iteration order is lexicographically sorted by coordinates (via
tagged_coords.sort()comparison)This implicitly documents the iteration order contract, which is useful for users relying on deterministic traversal.
ext/crates/once/benches/criterion/benchable_impl.rs (5)
22-27: LGTM!The iteration implementation correctly produces coordinate-value pairs for benchmarking. The use of
enumerate()works because benchmarks usemin = [0].
55-64: LGTM!The nested
flat_mapcorrectly produces 2D coordinates. Themoveclosure properly captures the outer index.
97-108: LGTM!Three-level nesting follows the established pattern consistently.
146-159: LGTM!Four-level nesting maintains consistency with the established pattern.
202-217: LGTM!Five-level nesting completes the benchable implementations. The author's PR comments note this approach was chosen over a recursive alternative due to better performance characteristics for typical K=2 usage.
ext/crates/once/src/grove/mod.rs (5)
363-369: LGTM!The iteration correctly handles Grove's sparse storage by filtering unset indices. Note that for very sparse Groves, this iterates through all indices up to
len()even if most are empty—acceptable given the data structure's design and use cases.
378-413: LGTM!Standard trait implementations are well-structured:
Debugformats as a map showing index-value pairsCloneproperly creates an independent copyPartialEq/Hashcorrectly include indices to distinguish sparse positionsNote that these traits don't have special handling for concurrent access—comparing or hashing during concurrent insertion may see inconsistent states. This is acceptable given Grove's insert-only semantics and typical single-threaded trait usage patterns.
636-650: LGTM with observation.The iteration correctly reconstructs negative indices by negating the storage index. Note that
enumerate()yields non-negative indices first, then negative indices—not sorted by value. This is fine for the intended use cases (Debug, Clone, PartialEq, Hash).
733-768: LGTM!Trait implementations mirror Grove's approach, correctly using
enumerate()and handling the two-ended index space through the existinginsert()routing logic.
811-839: LGTM!Tests effectively validate the new trait implementations:
- Debug test uses snapshot testing for stable output verification
- Clone test confirms the copy is distinct (
&raw constcomparison) yet equal in contentext/crates/once/benches/criterion/main.rs (5)
96-120: LGTM!The
KdTriebenchable implementation correctly adapts the trie's slice-based API to the array-basedBenchabletrait. Thetry_into().unwrap()initer()is safe sinceKdTrieguarantees all coordinates have exactlyKelements.
39-44: LGTM!The trait method signature correctly expresses the lifetime relationship between the iterator and borrowed self.
343-381: LGTM!Iteration benchmarks follow the established pattern, covering all available implementations across dimensions 1-6. The structure mirrors insert and lookup benchmarks for consistency.
401-425: LGTM!The iteration benchmark correctly exercises the full iteration path. Unlike lookup benchmarks, there's no shuffling since iteration order is determined by the data structure, not the caller.
211-227: LGTM!The function signatures cleanly extend to support a third benchable type, maintaining consistency across insert, lookup, and iteration benchmarks.
| /// Trait for managing coordinates during iteration | ||
| pub(super) trait Coordinates { | ||
| fn set_coord(&mut self, depth: usize, value: i32); | ||
| fn truncate_to(&mut self, depth: usize); | ||
| fn get(&self) -> Self; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting the Coordinates trait contract.
The trait assumes a specific usage pattern: truncate_to(depth) is called before set_coord(depth, value). For Vec<i32>, this is enforced by an assert in set_coord. Adding a brief doc comment about this contract would help future maintainers.
🔎 Proposed documentation
/// Trait for managing coordinates during iteration
+///
+/// Implementations must ensure that `set_coord(depth, value)` is only called
+/// after `truncate_to(depth)` has been called, so that the coordinate at
+/// `depth` can be set correctly.
pub(super) trait Coordinates {
fn set_coord(&mut self, depth: usize, value: i32);
fn truncate_to(&mut self, depth: usize);
fn get(&self) -> Self;
}🤖 Prompt for AI Agents
In ext/crates/once/src/multiindexed/iter.rs around lines 35 to 40, the
Coordinates trait lacks a doc comment describing the required usage contract:
callers must call truncate_to(depth) before set_coord(depth, value), and
implementations should either enforce this (e.g., Vec<i32> currently asserts in
set_coord) or document their behavior for out-of-order calls; add a concise doc
comment above the trait (and brief one-line comments on set_coord and
truncate_to) stating this contract, the expected preconditions, and that get
returns a copy of the current coordinate state.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ext/crates/once/src/multiindexed/kdtrie.rs (1)
210-216: Hash impl should includedimensionsfor consistency with PartialEq.This was flagged in a previous review: the
PartialEqimplementation checksself.dimensions != other.dimensions, butHashdoesn't includedimensions. While this technically doesn't violate thea == bimplieshash(a) == hash(b)invariant (since different dimensions means not equal), includingdimensionsin the hash is best practice for consistency.ext/crates/once/src/multiindexed/iter.rs (1)
35-40: Consider documenting theCoordinatestrait contract.The trait assumes a specific usage pattern:
truncate_to(depth)is called beforeset_coord(depth, value). ForVec<i32>, this is enforced by an assert inset_coord. Adding a brief doc comment about this contract would help future maintainers.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
ext/crates/once/Cargo.toml(1 hunks)ext/crates/once/benches/criterion/benchable_impl.rs(5 hunks)ext/crates/once/benches/criterion/main.rs(8 hunks)ext/crates/once/src/grove/mod.rs(5 hunks)ext/crates/once/src/multiindexed/iter.rs(1 hunks)ext/crates/once/src/multiindexed/kdtrie.rs(2 hunks)ext/crates/once/src/multiindexed/mod.rs(8 hunks)ext/crates/once/src/multiindexed/node.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-11T08:35:31.031Z
Learnt from: JoeyBF
Repo: SpectralSequences/sseq PR: 205
File: ext/crates/fp/src/limb.rs:11-28
Timestamp: 2025-12-11T08:35:31.031Z
Learning: In Rust 2024 edition (Cargo.toml edition = 2024), the mem::size_of, size_of_val, align_of, and align_of_val functions from std::mem are in the prelude and do not require explicit imports. During reviews, these can be used unqualified when the project targets edition = 2024. Verify the crate's Cargo.toml edition setting before assuming unqualified use, and avoid adding extraneous imports for these helpers.
Applied to files:
ext/crates/once/src/multiindexed/node.rsext/crates/once/src/multiindexed/iter.rsext/crates/once/benches/criterion/benchable_impl.rsext/crates/once/src/multiindexed/kdtrie.rsext/crates/once/src/grove/mod.rsext/crates/once/src/multiindexed/mod.rsext/crates/once/benches/criterion/main.rs
📚 Learning: 2025-12-14T08:23:53.399Z
Learnt from: JoeyBF
Repo: SpectralSequences/sseq PR: 207
File: ext/crates/fp/src/simd/x86_64/avx512.rs:367-379
Timestamp: 2025-12-14T08:23:53.399Z
Learning: In Rust 2024 edition, do not remove explicit unsafe blocks inside unsafe fn bodies. The lint unsafe_op_in_unsafe_fn is enabled by default, treating the body of an unsafe fn as safe and requiring each unsafe operation to be wrapped in an explicit unsafe { ... } block. Do not suggest removing these blocks in code using edition = "2024"; ensure unsafe operations are enclosed in explicit unsafe blocks and review for unnecessary nesting.
Applied to files:
ext/crates/once/src/multiindexed/node.rsext/crates/once/src/multiindexed/iter.rsext/crates/once/benches/criterion/benchable_impl.rsext/crates/once/src/multiindexed/kdtrie.rsext/crates/once/src/grove/mod.rsext/crates/once/src/multiindexed/mod.rsext/crates/once/benches/criterion/main.rs
🧬 Code graph analysis (3)
ext/crates/once/benches/criterion/benchable_impl.rs (4)
ext/crates/once/src/multiindexed/kdtrie.rs (1)
iter(164-170)ext/crates/once/src/multiindexed/mod.rs (1)
iter(226-228)ext/crates/once/benches/criterion/main.rs (4)
iter(41-43)iter(63-68)iter(88-93)iter(113-119)ext/crates/once/src/once.rs (2)
iter(381-386)iter(714-716)
ext/crates/once/src/multiindexed/kdtrie.rs (2)
ext/crates/once/src/multiindexed/mod.rs (3)
iter(226-228)new(101-103)new(395-398)ext/crates/once/src/multiindexed/iter.rs (2)
new(20-32)new(55-61)
ext/crates/once/benches/criterion/main.rs (3)
ext/crates/once/benches/criterion/benchable_impl.rs (25)
iter(22-27)iter(55-61)iter(94-102)iter(140-148)iter(191-199)name(6-8)name(31-33)name(65-67)name(106-108)name(152-154)new(10-12)new(35-37)new(69-71)new(110-112)new(156-158)push_checked(14-16)push_checked(39-42)push_checked(73-77)push_checked(114-119)push_checked(160-166)get(18-20)get(44-53)get(79-92)get(121-138)get(168-189)ext/crates/once/src/multiindexed/kdtrie.rs (3)
iter(164-170)new(45-55)get(71-82)ext/crates/once/src/multiindexed/mod.rs (4)
iter(226-228)new(101-103)new(395-398)get(150-152)
⏰ 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). (11)
- GitHub Check: webserver (nightly)
- GitHub Check: webserver (beta)
- GitHub Check: webserver (stable)
- GitHub Check: test (nightly)
- GitHub Check: test (stable)
- GitHub Check: calculator (nightly)
- GitHub Check: test (beta)
- GitHub Check: miri
- GitHub Check: calculator (beta)
- GitHub Check: calculator (stable)
- GitHub Check: build
🔇 Additional comments (31)
ext/crates/once/Cargo.toml (1)
18-18: LGTM!The
expect-testcrate is a well-established snapshot testing library maintained by the rust-analyzer team. It's appropriately added as a dev-dependency for verifying debug output in the new tests.ext/crates/once/src/grove/mod.rs (5)
363-369: LGTM!The
iter()andenumerate()methods are well-implemented. Theenumerate()correctly filters out unset indices by usingfilter_mapwithself.get(), ensuring only populated entries are yielded.
378-413: LGTM!The trait implementations for
Grove<T>are consistent and correct:
Debugformats as a map, which is appropriate for a sparse structureClonecorrectly preserves the sparse structure by iterating and reinsertingPartialEqandHashare consistent—both operate on the same(index, value)pairs
636-650: LGTM!The
iter()andenumerate()methods correctly chain the non-negative and negative groves. The iteration order (non-negative indices first, then negative) is deterministic, ensuring consistent behavior forPartialEqandHashcomparisons.
733-768: LGTM!The trait implementations for
TwoEndedGrove<T>follow the same consistent pattern asGrove<T>and are correctly implemented.
811-839: LGTM!Good test coverage for the new trait implementations. The
test_grove_debugusesexpect_testfor readable snapshot testing, andtest_grove_cloneproperly verifies that cloning creates a distinct instance with equal content using raw pointer comparison.ext/crates/once/src/multiindexed/node.rs (1)
163-179: LGTM!The new
inner()andleaf()accessors are well-documented and correctly use explicitunsafeblocks within the unsafe function bodies, as required by Rust 2024 edition's defaultunsafe_op_in_unsafe_fnlint. These accessors enable the iteration functionality inKdIterator.ext/crates/once/src/multiindexed/kdtrie.rs (2)
159-174: LGTM!The new public API methods are well-implemented:
dimensions()provides a straightforward accessoriter()correctly initializesKdIteratorwith a pre-allocatedVecfor coordinatesroot()is appropriately scoped aspub(super)for internal iteration support
183-206: LGTM!The
Debug,Clone, andPartialEqimplementations are correct.Cloneproperly reconstructs the trie structure, andPartialEqcorrectly checks dimensions before comparing iteration order.ext/crates/once/benches/criterion/benchable_impl.rs (5)
21-27: LGTM!The
iter()implementation correctly maps enumerated items to coordinate-value pairs. The casti as i32handles the index type conversion appropriately.
54-61: LGTM!The recursive composition pattern using
Benchable::<1, _>::iter(self)withflat_mapcorrectly traverses nestedOnceBiVecstructures and builds 2D coordinates.
93-102: LGTM!The level 3 implementation correctly extends the recursive pattern to 3D coordinates.
139-148: LGTM!Level 4 iteration correctly follows the established recursive pattern.
190-199: LGTM!Level 5 iteration completes the set with consistent implementation.
ext/crates/once/src/multiindexed/mod.rs (9)
1-6: LGTM!Module organization is clean. The
itermodule is appropriately private whileKdIteratoris imported for internal use.
207-228: LGTM!The
iter()method is well-documented with a clear example. Using[0; K]as the initial coordinate array is efficient since arrays areCopy.
243-254: Clone impl may produce different iteration order than the original.The
Cloneimplementation iterates overselfand inserts into a newMultiIndexed. Since concurrent insertions inMultiIndexedcan result in different internal tree structures depending on insertion order, and the iteration order depends on the tree structure,cloned.iter()may yield elements in a different order thanself.iter(). This is fine for the data structure semantics but worth noting in documentation if iteration order stability matters to users.The implementation is correct for the use case.
256-263: PartialEq relies on iteration order—verify this is intended.The
PartialEqimplementation usesself.iter().eq(other.iter()), which compares elements in iteration order. This means twoMultiIndexedinstances with the same key-value pairs but different internal structures (due to different insertion orders) would compare as unequal if their iteration orders differ.If the intent is value equality regardless of insertion history, consider collecting into sorted vectors or using a set-based comparison. If order-sensitive equality is intended (which seems likely given the trie structure), this is correct.
267-274: Hash impl is order-dependent—ensure consistency with PartialEq.The
Hashimplementation iterates and hashes each(coords, value)pair. This is consistent with thePartialEqimplementation (both are order-dependent), which satisfies the requirement thata == bimplieshash(a) == hash(b).However, if two structurally different
MultiIndexedinstances with the same logical content produce different iteration orders, they would have different hashes but might be considered "logically equal" by users. This is acceptable if the equality semantics are well-documented.
360-378: LGTM!Good test coverage for edge cases: empty iteration and multiple calls returning consistent results.
434-476: LGTM!The
Debugtest usesexpect_testfor snapshot testing, which is a clean approach. TheClonetest verifies both that values are preserved and that missing keys returnNone.
568-606: LGTM!The property tests are well-structured:
proptest_multiindexed_ops_kdvalidates get/insert against a HashMap referenceproptest_multiindexed_iter_kdvalidates iteration by comparing sorted resultsThe sorting before comparison in
proptest_multiindexed_iter_kdis the right approach since it tests correctness without depending on iteration order.
486-490: Clarify the relationship between max_len and hypercube_size in the code comment.The formula correctly limits hypercube coordinates, but the comment is inverted. For
max_len = 10_000andK = 2, the resulting hypercube size is ~5,300 (a ratio of 0.5), not 20,000. The code comment should state that the hypercube size is approximately half of max_len, or that max_len is approximately twice the hypercube size—whichever reflects the intended design.ext/crates/once/src/multiindexed/iter.rs (3)
64-129: Iterator logic is correct but dense—consider adding inline comments for clarity.The DFS traversal logic correctly:
- Pops frames from the stack
- Truncates coordinates to current depth
- Iterates through the range to find valid entries
- Pushes remaining range back before returning/descending
- Handles leaf vs inner nodes based on depth
The unsafe calls are correctly guarded by depth checks that match the tree structure invariants.
One minor observation: the
breakat line 121 after pushing a child frame is intentional to continue the outerwhileloop—this is correct but could benefit from a comment.🔎 Suggested clarifying comment
self.stack.push(IterFrame { depth: depth + 1, current_node: child_node, range: child_range, }); - // Go to the next iteration of the outer loop, which will process the child + // Break inner loop to process the newly-pushed child frame in the outer loop break;
131-143: LGTM!The
[i32; K]implementation is efficient:
set_coorddirectly indexes into the arraytruncate_tois a no-op (correct since the array is fixed-size and old values get overwritten)getreturns a copy (arrays areCopy)
145-158: Assert inset_coordenforces the contract—good defensive programming.The
assert_eq!(self.len(), depth)ensurestruncate_towas called beforeset_coord, preventing subtle bugs. Theclone()inget()is necessary forVecbut noted in the struct docs as the reason fixed arrays provide 3× speedup.ext/crates/once/benches/criterion/main.rs (5)
6-6: LGTM!Import added for the newly benchable
KdTrietype.
39-44: LGTM!The
itermethod signature correctly uses RPITIT (return position impl trait in trait) with the appropriate lifetime boundT: 'a.
96-120: LGTM!The
KdTrieimplementation correctly adapts theVec<i32>coordinates to[i32; K]usingtry_into().unwrap(). The unwrap is safe becauseKdTrie::new(K)ensures the iterator produces vectors of exactly lengthK.
211-227: Good generalization to support three benchable types.Adding the
B3type parameter cleanly extends the benchmark harness to includeKdTriealongside existing types.
340-425: LGTM!The iteration benchmarks follow the same pattern as lookup benchmarks:
- Pre-populate the container
- Benchmark iterating through all elements with
black_boxThe structure mirrors the existing insert/lookup benchmarks for consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
ext/crates/once/benches/criterion/benchable_impl.rsext/crates/once/benches/criterion/main.rsext/crates/once/src/multiindexed/iter.rsext/crates/once/src/multiindexed/kdtrie.rsext/crates/once/src/multiindexed/mod.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-11T08:35:31.031Z
Learnt from: JoeyBF
Repo: SpectralSequences/sseq PR: 205
File: ext/crates/fp/src/limb.rs:11-28
Timestamp: 2025-12-11T08:35:31.031Z
Learning: In Rust 2024 edition (Cargo.toml edition = 2024), the mem::size_of, size_of_val, align_of, and align_of_val functions from std::mem are in the prelude and do not require explicit imports. During reviews, these can be used unqualified when the project targets edition = 2024. Verify the crate's Cargo.toml edition setting before assuming unqualified use, and avoid adding extraneous imports for these helpers.
Applied to files:
ext/crates/once/src/multiindexed/kdtrie.rsext/crates/once/benches/criterion/benchable_impl.rsext/crates/once/benches/criterion/main.rsext/crates/once/src/multiindexed/mod.rsext/crates/once/src/multiindexed/iter.rs
📚 Learning: 2025-12-14T08:23:53.399Z
Learnt from: JoeyBF
Repo: SpectralSequences/sseq PR: 207
File: ext/crates/fp/src/simd/x86_64/avx512.rs:367-379
Timestamp: 2025-12-14T08:23:53.399Z
Learning: In Rust 2024 edition, do not remove explicit unsafe blocks inside unsafe fn bodies. The lint unsafe_op_in_unsafe_fn is enabled by default, treating the body of an unsafe fn as safe and requiring each unsafe operation to be wrapped in an explicit unsafe { ... } block. Do not suggest removing these blocks in code using edition = "2024"; ensure unsafe operations are enclosed in explicit unsafe blocks and review for unnecessary nesting.
Applied to files:
ext/crates/once/src/multiindexed/kdtrie.rsext/crates/once/benches/criterion/benchable_impl.rsext/crates/once/benches/criterion/main.rsext/crates/once/src/multiindexed/mod.rsext/crates/once/src/multiindexed/iter.rs
🧬 Code graph analysis (4)
ext/crates/once/src/multiindexed/kdtrie.rs (2)
ext/crates/once/src/multiindexed/mod.rs (6)
fmt(215-217)clone(224-230)new(100-102)new(372-375)eq(237-239)hash(245-250)ext/crates/once/src/multiindexed/iter.rs (2)
new(21-36)new(59-65)
ext/crates/once/benches/criterion/benchable_impl.rs (3)
ext/crates/once/src/multiindexed/iter.rs (2)
iter(69-72)iter(94-96)ext/crates/once/src/grove/mod.rs (2)
iter(363-365)iter(636-638)ext/crates/once/src/once.rs (2)
iter(381-386)iter(714-716)
ext/crates/once/src/multiindexed/mod.rs (2)
ext/crates/once/src/multiindexed/iter.rs (4)
iter(69-72)iter(94-96)new(21-36)new(59-65)ext/crates/once/src/multiindexed/kdtrie.rs (2)
fmt(176-178)new(45-55)
ext/crates/once/src/multiindexed/iter.rs (2)
ext/crates/once/src/multiindexed/kdtrie.rs (4)
new(45-55)dimensions(160-162)root(164-166)get(71-82)ext/crates/once/src/multiindexed/mod.rs (3)
new(100-102)new(372-375)get(149-151)
⏰ 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). (9)
- GitHub Check: test (nightly)
- GitHub Check: test (beta)
- GitHub Check: test (stable)
- GitHub Check: webserver (beta)
- GitHub Check: webserver (nightly)
- GitHub Check: docs (stable)
- GitHub Check: webserver (stable)
- GitHub Check: miri
- GitHub Check: lint
🔇 Additional comments (14)
ext/crates/once/src/multiindexed/iter.rs (2)
68-73: LGTM! Well-designed iterator constructor.The iteration infrastructure is well-designed, using a generic Coordinates trait to abstract over both fixed-size arrays (Copy, 3× faster per PR description) and dynamic vectors. The constructor properly initializes the stack with the root frame.
99-164: LGTM! Solid DFS iteration implementation.The iterator correctly implements depth-first traversal with proper coordinate tracking, frame management, and safe handling of both inner and leaf nodes. The unsafe calls are justified by the depth comparisons ensuring correct node types.
ext/crates/once/src/multiindexed/kdtrie.rs (2)
160-166: LGTM! Clean accessor methods.The
dimensions()androot()accessors properly expose internal state needed for iteration without compromising encapsulation. Thepub(super)visibility onroot()is appropriately restrictive.
175-207: LGTM! Well-implemented standard traits.All trait implementations correctly leverage the new iterator:
Debugformats as a map of coordinates to valuesClonerebuilds from iteration (correct for write-once semantics)PartialEqcompares dimensions first for early exit, then compares contentHashcorrectly includes dimensions (line 201) to distinguish empty triesThe implementations are consistent with similar patterns in
MultiIndexed.ext/crates/once/benches/criterion/benchable_impl.rs (2)
22-27: LGTM! Clean base case for recursive iteration pattern.The Level 1 iterator correctly maps
iter_enum()to produce the required([i32; 1], &T)signature, establishing the base case for the recursive pattern used in higher dimensions.
55-61: LGTM! Clever recursive composition.The recursive pattern (
Benchable::<K-1, _>::itercomposed withflat_map) elegantly builds K-dimensional iteration from (K-1)-dimensional iteration. While more complex than direct implementation, this approach promotes code reuse across nesting levels.ext/crates/once/src/multiindexed/mod.rs (6)
3-4: LGTM! Appropriate module visibility changes.Making
kdtriepublic and adding theitermodule correctly exposes the iteration infrastructure while maintaining encapsulation.
214-251: LGTM! Complete and correct standard trait implementations.All trait implementations are well-designed:
Debugusesdebug_map()for readable formattingClonecorrectly rebuilds from iteration (appropriate for write-once semantics)PartialEqandEqcompare content via iterator equalityHashhashes all coordinate-value pairs in iteration orderNote:
Kdoesn't need explicit hashing/comparison since it's a const generic type parameter—differentKvalues yield incompatible types.
337-355: LGTM! Good test coverage for iterator edge cases.The tests correctly verify empty iteration and idempotency of multiple
iter()calls, covering important edge cases for the new iterator implementation.
411-453: LGTM! Solid integration tests for trait implementations.The
test_debugusesexpect_testto verify formatting output (ensuring deterministic iteration order), andtest_cloneconfirms the cloned structure has identical contents. Good coverage of the new trait implementations.
463-514: LGTM! Refined property test strategy.The introduction of
max_from_max_lento compute coordinate bounds based on hypercube volume is a thoughtful improvement that better calibrates test coverage for different dimensionalities.
545-583: LGTM! Comprehensive property tests for iteration.The
proptest_multiindexed_iter_kdtest validates:
- Iterator yields exactly the inserted coordinates
- Iteration order is deterministic (matches sorted coordinates)
- No duplicates or missing entries
This provides strong assurance of iteration correctness across different dimensions and coordinate patterns.
ext/crates/once/benches/criterion/main.rs (2)
40-43: LGTM! Clean trait extension for iteration benchmarks.The addition of the
itermethod to theBenchabletrait maintains consistency with existinggetandpush_checkedmethods, enabling uniform benchmarking across all container types.
343-425: LGTM! Comprehensive iteration benchmarks.The iteration benchmark suite mirrors the structure of insert and lookup benchmarks, providing consistent coverage across dimensions 1–6 and all three container types (OnceBiVec, MultiIndexed, KdTrie). This enables direct performance comparison of the new iteration paths.
| // Safety: This function is only called by KdIterator::new, which is only called by the iter | ||
| // methods of KdTrie and MultiIndexed. Therefore, by definition, the number of dimensions | ||
| // can be trusted. There can not be any other caller because of the pub(self) visibility. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Correct the visibility reference in the safety comment.
The comment states "pub(self) visibility" but KdIterator is declared with pub(super) visibility on line 52. Update the comment to accurately reflect the actual visibility.
🔎 Proposed fix
- // Safety: This function is only called by KdIterator::new, which is only called by the iter
- // methods of KdTrie and MultiIndexed. Therefore, by definition, the number of dimensions
- // can be trusted. There can not be any other caller because of the pub(self) visibility.
+ // Safety: This function is only called by KdIterator::new, which is only called by the iter
+ // methods of KdTrie and MultiIndexed. Therefore, by definition, the number of dimensions
+ // can be trusted. There can not be any other caller because of the pub(super) visibility.📝 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.
| // Safety: This function is only called by KdIterator::new, which is only called by the iter | |
| // methods of KdTrie and MultiIndexed. Therefore, by definition, the number of dimensions | |
| // can be trusted. There can not be any other caller because of the pub(self) visibility. | |
| // Safety: This function is only called by KdIterator::new, which is only called by the iter | |
| // methods of KdTrie and MultiIndexed. Therefore, by definition, the number of dimensions | |
| // can be trusted. There can not be any other caller because of the pub(super) visibility. |
🤖 Prompt for AI Agents
In ext/crates/once/src/multiindexed/iter.rs around lines 22 to 24, the safety
comment incorrectly references "pub(self) visibility" but KdIterator is declared
with pub(super) visibility on line 52; update the comment to reference
"pub(super) visibility" so the visibility statement matches the actual
declaration, keeping the rest of the safety rationale unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ext/crates/once/src/multiindexed/iter.rs (2)
22-24: Correct the visibility reference in the safety comment.The comment references "pub(self) visibility" but
KdIteratoris declared withpub(super)visibility. The safety rationale is sound, but the visibility reference should be updated for accuracy.🔎 Proposed fix
// Safety: This function is only called by KdIterator::new, which is only called by the iter // methods of KdTrie and MultiIndexed. Therefore, by definition, the number of dimensions - // can be trusted. There can not be any other caller because of the pub(self) visibility. + // can be trusted. There can not be any other caller because of the pub(super) visibility.
39-44: Consider documenting theCoordinatestrait contract.The trait has an implicit usage contract:
truncate_to(depth)must be called beforeset_coord(depth, value). TheVec<i32>implementation enforces this via an assertion (line 182), but the trait itself lacks documentation explaining this requirement.🔎 Proposed documentation
/// Trait for managing coordinates during iteration +/// +/// Implementations must ensure that `set_coord(depth, value)` is only called +/// after `truncate_to(depth)` has been called, so that the coordinate at +/// `depth` can be set correctly. trait Coordinates { fn set_coord(&mut self, depth: usize, value: i32); fn truncate_to(&mut self, depth: usize); fn get(&self) -> Self; }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
ext/crates/once/benches/criterion/main.rsext/crates/once/src/multiindexed/iter.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-11T08:35:31.031Z
Learnt from: JoeyBF
Repo: SpectralSequences/sseq PR: 205
File: ext/crates/fp/src/limb.rs:11-28
Timestamp: 2025-12-11T08:35:31.031Z
Learning: In Rust 2024 edition (Cargo.toml edition = 2024), the mem::size_of, size_of_val, align_of, and align_of_val functions from std::mem are in the prelude and do not require explicit imports. During reviews, these can be used unqualified when the project targets edition = 2024. Verify the crate's Cargo.toml edition setting before assuming unqualified use, and avoid adding extraneous imports for these helpers.
Applied to files:
ext/crates/once/benches/criterion/main.rsext/crates/once/src/multiindexed/iter.rs
📚 Learning: 2025-12-14T08:23:53.399Z
Learnt from: JoeyBF
Repo: SpectralSequences/sseq PR: 207
File: ext/crates/fp/src/simd/x86_64/avx512.rs:367-379
Timestamp: 2025-12-14T08:23:53.399Z
Learning: In Rust 2024 edition, do not remove explicit unsafe blocks inside unsafe fn bodies. The lint unsafe_op_in_unsafe_fn is enabled by default, treating the body of an unsafe fn as safe and requiring each unsafe operation to be wrapped in an explicit unsafe { ... } block. Do not suggest removing these blocks in code using edition = "2024"; ensure unsafe operations are enclosed in explicit unsafe blocks and review for unnecessary nesting.
Applied to files:
ext/crates/once/benches/criterion/main.rsext/crates/once/src/multiindexed/iter.rs
🧬 Code graph analysis (1)
ext/crates/once/src/multiindexed/iter.rs (2)
ext/crates/once/src/multiindexed/kdtrie.rs (4)
new(45-55)dimensions(160-162)root(164-166)get(71-82)ext/crates/once/src/multiindexed/mod.rs (3)
new(100-102)new(372-375)get(149-151)
⏰ 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). (6)
- GitHub Check: test (nightly)
- GitHub Check: test (stable)
- GitHub Check: test (beta)
- GitHub Check: webserver (nightly)
- GitHub Check: webserver (beta)
- GitHub Check: miri
🔇 Additional comments (10)
ext/crates/once/benches/criterion/main.rs (5)
40-43: LGTM: Iteration API added to Benchable trait.The
itermethod signature is well-designed with appropriate lifetime bounds, enabling generic iteration across all benchable types.
96-126: LGTM: KdTrie Benchable implementation is correct.The
expectmessage on lines 120-121 properly documents the invariant that KdTrie guarantees all coordinates have length K, making thetry_into()conversion safe. This addresses the previous review comment by providing clear context that will appear in panic output.
186-232: LGTM: KdTrie consistently integrated into insertion benchmarks.The addition of the
B3type parameter and KdTrie integration across all dimension levels is systematic and follows the existing pattern.
270-317: LGTM: KdTrie consistently integrated into lookup benchmarks.The changes mirror the insertion benchmark structure, maintaining consistency across the benchmark suite.
347-431: LGTM: Iteration benchmarks are well-structured and comprehensive.The iteration benchmark infrastructure follows the same systematic approach as insertion and lookup benchmarks, covering dimensions 1-6 and properly using
black_boxto ensure accurate performance measurement. The implementation aligns with the PR objectives to verify near-optimal iteration performance.ext/crates/once/src/multiindexed/iter.rs (5)
6-17: LGTM: IterFrame is well-documented and appropriately designed.The frame structure effectively captures the state needed for depth-first traversal of the multi-dimensional tree.
46-66: LGTM: Excellent documentation on the performance rationale.The doc comment clearly explains why the generic coordinate abstraction exists and quantifies the performance benefit (3× speedup) of using fixed-size arrays. This provides valuable context for future maintainers.
68-73: LGTM: Efficient KdTrie iteration implementation.The use of
Vec::with_capacity(dimensions)is a good optimization that avoids reallocations during iteration.
75-97: LGTM: Well-documented MultiIndexed iteration API.The comprehensive doc comment with a working example makes this API immediately usable. The use of fixed-size arrays leverages the
Copytrait for better performance compared toVec<i32>.
166-193: LGTM: Coordinates implementations are correct and efficient.The array implementation avoids allocations by being a no-op for
truncate_to, while theVecimplementation enforces the trait contract via the assertion on line 182. Both implementations correctly support the iteration logic.
* Add iteration methods to `MultiIndexed` et al. * Better iteration abstractions * Better bench * Impl standard traits on `MultiIndexed` * Impl standard traits on `Grove` too * Fix unsafe in unsafe fn * Improve `Benchable::iter` for nested `OnceVec`s * Simplify `PartialEq` impl for `KdTrie` * Fix `Hash` impl for `KdTrie` * Make `Coordinates` one-off trait completely private * Make `KdIterator` more private * Format * Fix iter lint * Make `KdIterator` properly private * Replace unwrap by expect
* Add iteration methods to `MultiIndexed` et al. * Better iteration abstractions * Better bench * Impl standard traits on `MultiIndexed` * Impl standard traits on `Grove` too * Fix unsafe in unsafe fn * Improve `Benchable::iter` for nested `OnceVec`s * Simplify `PartialEq` impl for `KdTrie` * Fix `Hash` impl for `KdTrie` * Make `Coordinates` one-off trait completely private * Make `KdIterator` more private * Format * Fix iter lint * Make `KdIterator` properly private * Replace unwrap by expect
1 participant
It was getting annoying that
MultiIndexeddidn't implement Debug, Clone, PartialEq, Eq, Hash, since any structs that contain one would not be able to derive them. The main bottleneck was being able to iterate over the entire tree.I implemented iterators and updated the criterion bench to make sure the performance is close to optimal. I also implemented all the standard traits for
MultiIndexed,KdTrie,GroveandTwoSidedGrove. I added proptests to check for correctness too.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.