Conversation
There was a problem hiding this comment.
Pull request overview
This PR targets faster ISO 6709 parsing by replacing str::parse()-based numeric parsing with byte-level helpers, and updates benchmark/documentation artifacts to reflect the improved results.
Changes:
- Introduces fast numeric parsing helpers (
digit2,digit3,fast_parse_f64) and uses them in latitude/longitude/altitude parsing. - Disables Criterion benchmarks under Miri to avoid extremely slow runs.
- Updates README benchmark numbers based on the new performance results.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/iso6709/mod.rs |
Adds fast numeric parsing helpers and switches coordinate component parsing to use them. |
benches/parse.rs |
Skips compiling/running benchmarks under Miri. |
README.md |
Updates published benchmark results and batch throughput numbers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| /// Fast f64 parser for regex-validated `DIGITS[.DIGITS]` strings. | ||
| /// | ||
| /// Avoids the full `str::parse::<f64>()` which handles scientific notation, | ||
| /// infinity, NaN, etc. Since logos validates the format, we only need simple | ||
| /// integer + optional fractional digit parsing. | ||
| #[cfg_attr(not(tarpaulin), inline(always))] | ||
| fn fast_parse_f64(b: &[u8]) -> f64 { | ||
| match memchr_dot(b) { | ||
| None => { | ||
| let mut val: u64 = 0; | ||
| for &byte in b { | ||
| val = val * 10 + (byte - b'0') as u64; | ||
| } | ||
| val as f64 | ||
| } | ||
| Some(dot) => { | ||
| let mut int_val: u64 = 0; | ||
| for &byte in &b[..dot] { | ||
| int_val = int_val * 10 + (byte - b'0') as u64; | ||
| } | ||
| let frac_bytes = &b[dot + 1..]; | ||
| let mut frac_val: u64 = 0; | ||
| for &byte in frac_bytes { | ||
| frac_val = frac_val * 10 + (byte - b'0') as u64; | ||
| } | ||
| // Use a lookup table for common fractional lengths to avoid pow(). | ||
| let divisor = match frac_bytes.len() { | ||
| 1 => 10.0, | ||
| 2 => 100.0, | ||
| 3 => 1_000.0, | ||
| 4 => 10_000.0, | ||
| 5 => 100_000.0, | ||
| 6 => 1_000_000.0, | ||
| n => 10f64.powi(n as i32), | ||
| }; | ||
| int_val as f64 + frac_val as f64 / divisor | ||
| } |
There was a problem hiding this comment.
There are many parsing tests in this file, but none that exercise very long signed numeric inputs (especially altitude via SignedOther). Since the new fast_parse_f64 implementation has overflow/precision edge cases compared to str::parse, adding a regression test that parses an altitude with >19 digits (and asserts it does not panic) would help lock in behavior.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
No description provided.