perf: optimize scale finding algorithm from O(n) to O(1) #9
Conversation
WalkthroughReplaced an iterative scale search in src/get_las_writer.rs with a closed-form calculation using MIN_SCALE and QUANTUM. Added unit tests for the scale function. Updated Cargo.toml to include an empty [dev-dependencies] section and set codegen-units = 1 under [profile.release]. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant Writer as get_las_writer
participant Scale as find_smallest_scale
Caller->>Writer: request LAS writer for value x
activate Writer
Writer->>Scale: compute smallest_scale(x)
activate Scale
alt |x| <= i32::MAX * MIN_SCALE
Scale-->>Writer: return MIN_SCALE
else |x| > threshold
Scale-->>Writer: return ceil((|x|/i32::MAX)/QUANTUM)*QUANTUM
end
deactivate Scale
Writer-->>Caller: constructed writer with scale
deactivate Writer
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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. 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: 0
🧹 Nitpick comments (8)
Cargo.toml (1)
36-38: Bench target config: consider minor QoL tweaksOptional:
- Enable HTML reports for clearer output.
- Name the group consistently with the file.
Suggested change:
[[bench]] name = "scale_finding" harness = false +## optional: nicer reports +# criterion is dev-only; enabling reports won’t affect the library +# run with: cargo benchsrc/get_las_writer.rs (4)
15-24: Replace magic 10000.0 with a quantization constant; keep math identicalImproves readability and keeps benches/tests in sync with the same quantum.
- let theoretical_min = x.abs() / f64::from(i32::MAX); - - let scale = (theoretical_min * 10000.0).ceil() / 10000.0; - - scale.max(MIN_SCALE) + let theoretical_min = x.abs() / f64::from(i32::MAX); + // Quantize to 0.0001 steps, matching the legacy increment. + const QUANTUM: f64 = 1e-4; + let scale = ((theoretical_min / QUANTUM).ceil()) * QUANTUM; + scale.max(MIN_SCALE)
60-71: Tests: avoid repeating the magic 0.001 and tie to a single constantHoist MIN_SCALE to module scope so tests and impl share the same value, preventing drift.
Patch (also updates the function to use the shared constant):
-use crate::LasVersion; +use crate::LasVersion; + +const MIN_SCALE: f64 = 0.001; @@ -fn find_smallest_scale(x: f64) -> f64 { - const MIN_SCALE: f64 = 0.001; +fn find_smallest_scale(x: f64) -> f64 { @@ - assert_eq!(find_smallest_scale(0.0), 0.001); - assert_eq!(find_smallest_scale(1.0), 0.001); - assert_eq!(find_smallest_scale(1000.0), 0.001); - assert_eq!(find_smallest_scale(-1000.0), 0.001); + assert_eq!(find_smallest_scale(0.0), MIN_SCALE); + assert_eq!(find_smallest_scale(1.0), MIN_SCALE); + assert_eq!(find_smallest_scale(1000.0), MIN_SCALE); + assert_eq!(find_smallest_scale(-1000.0), MIN_SCALE); @@ - let max_with_min_scale = f64::from(i32::MAX) * 0.001; - assert_eq!(find_smallest_scale(max_with_min_scale), 0.001); + let max_with_min_scale = f64::from(i32::MAX) * MIN_SCALE; + assert_eq!(find_smallest_scale(max_with_min_scale), MIN_SCALE);
89-99: Boundary test: add a “half-step” case to pin rounding behaviorThis catches off-by-one due to .round() semantics near the threshold.
#[test] fn test_find_smallest_scale_boundary() { // Test value just above the threshold - let just_above = f64::from(i32::MAX) * 0.001 + 1.0; + let just_above = f64::from(i32::MAX) * MIN_SCALE + 1.0; let scale = find_smallest_scale(just_above); - assert!(scale > 0.001); + assert!(scale > MIN_SCALE); // Verify it still works let scaled = (just_above / scale).round(); assert!(scaled <= f64::from(i32::MAX)); + + // Half-step close to the threshold + let half_step = f64::from(i32::MAX) * MIN_SCALE + (MIN_SCALE / 2.0); + let s2 = find_smallest_scale(half_step); + let scaled2 = (half_step / s2).round(); + assert!(scaled2 <= f64::from(i32::MAX)); }
113-124: Precision test: include negatives and assert “scale >= theoretical_min”Strengthens invariants and symmetry.
- let test_values = [2.15e9, 3.7e9, 5.5e9, 1e11]; + let test_values = [2.15e9, 3.7e9, 5.5e9, 1e11, -2.15e9, -1e11]; @@ let scale = find_smallest_scale(value); // Check that scale has at most 4 decimal places let multiplied = scale * 10000.0; assert!((multiplied - multiplied.round()).abs() < 1e-10); + // And that it meets the theoretical lower bound + let theoretical_min = value.abs() / f64::from(i32::MAX); + assert!(scale + 1e-12 >= theoretical_min);benches/scale_finding.rs (3)
17-26: Keep constants/quantization aligned with src to avoid driftDefine MIN_SCALE/QUANTUM once in this bench file and reuse in both impls for readability and consistency.
-// New optimized implementation -fn find_smallest_scale_new(x: f64) -> f64 { - const MIN_SCALE: f64 = 0.001; +const MIN_SCALE: f64 = 0.001; +const QUANTUM: f64 = 1e-4; + +// New optimized implementation +fn find_smallest_scale_new(x: f64) -> f64 { if x.abs() <= f64::from(i32::MAX) * MIN_SCALE { return MIN_SCALE; } let theoretical_min = x.abs() / f64::from(i32::MAX); - let scale = (theoretical_min * 10000.0).ceil() / 10000.0; + let scale = ((theoretical_min / QUANTUM).ceil()) * QUANTUM; scale.max(MIN_SCALE) }
28-53: Broaden inputs (negatives) and assert parity once before timingAdds quick sanity to ensure both paths match before benchmarking.
- let test_values = [ - ("small", 1000.0), - ("medium", 2.15e9), - ("large", 1e10), - ("very_large", 1e11), - ]; + let test_values = [ + ("small", 1000.0), + ("medium", 2.15e9), + ("large", 1e10), + ("very_large", 1e11), + ("neg_medium", -2.15e9), + ("neg_very_large", -1e11), + ]; @@ - for (name, value) in test_values { + for (name, value) in test_values { + // Quick equivalence check (outside the hot loop) + assert!( + (find_smallest_scale_old(value) - find_smallest_scale_new(value)).abs() < 1e-12, + "old/new mismatch for {name}" + ); group.bench_with_input( BenchmarkId::new("old", name), &value, |b, &val| b.iter(|| find_smallest_scale_old(black_box(val))) );
55-79: Throughput bench: add throughput metadata and reuse the same dataset for both benchesThis improves report interpretability and avoids accidental optimizer bias.
fn benchmark_throughput(c: &mut Criterion) { - let mut group = c.benchmark_group("throughput"); + let mut group = c.benchmark_group("throughput"); - let values: Vec<f64> = (0..1000) + let values: Vec<f64> = (0..1000) .map(|i| 1e6 * (i as f64)) .collect(); + group.throughput(criterion::Throughput::Elements(values.len() as u64)); group.bench_function("old_algorithm", |b| { b.iter(|| { - for &val in &values { + for &val in black_box(&values) { find_smallest_scale_old(black_box(val)); } }) }); group.bench_function("new_algorithm", |b| { b.iter(|| { - for &val in &values { + for &val in black_box(&values) { find_smallest_scale_new(black_box(val)); } }) });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.toml(1 hunks)benches/scale_finding.rs(1 hunks)src/get_las_writer.rs(2 hunks)
🔇 Additional comments (3)
Cargo.toml (1)
28-30: Dev-only Criterion addition looks goodScoped under [dev-dependencies]; won’t bloat release builds.
src/get_las_writer.rs (1)
10-13: Add finite-input guard to find_smallest_scale (optional)If
max_cartesiancan be NaN or infinite (e.g. from corrupt E57 data), the current fast-path returns NaN/∞ scale. Consider adding at the top offind_smallest_scale:fn find_smallest_scale(x: f64) -> f64 { const MIN_SCALE: f64 = 0.001; + debug_assert!(x.is_finite(), "max_cartesian must be finite"); + if !x.is_finite() { + return MIN_SCALE; + } if x.abs() <= f64::from(i32::MAX) * MIN_SCALE { return MIN_SCALE; }Confirm whether the E57 reader always yields finite coordinates; if it does, this guard can be omitted.
benches/scale_finding.rs (1)
4-14: Old baseline retained for comparison — OKKeeping the slow path here is useful for parity checks and clarity in benches.
- Replace linear search with direct mathematical calculation - Add early return for small values that work with minimum scale - Calculate theoretical minimum scale based on i32 bounds - Add criterion benchmarks showing up to 447x speedup - Maintain functional equivalence with comprehensive tests
- Replace magic numbers with named constants (MIN_SCALE, QUANTUM) - Add helpful comments to Cargo.toml for benchmark usage - Improve test coverage with negative values and half-step boundary cases - Add theoretical minimum assertions to validate scale correctness - Align constants between implementation and benchmarks - Add throughput metadata to benchmarks for better reporting - Add equivalence checks in benchmarks to ensure parity
pnwatin
force-pushed
the
optimize-scale-finding
branch
from
2b780ce to
d2c68fa
Compare
Summary
Changes
Performance Improvements
Benchmark results using Criterion 0.5:
Testing
The optimization provides massive performance benefits especially for large coordinate values commonly found in real-world point cloud data.
Summary by CodeRabbit