perf: speed up parsing 2-4x by trimming redundant work in hot paths - #332
anasbekheit wants to merge 7 commits into
Conversation
|
This PR improves parsing speed (related coreutils PR), the combined effect of coreutils-14866 and this PR makes coreutils date perf match GNU's performance. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #332 +/- ##
==========================================
+ Coverage 97.48% 97.51% +0.03%
==========================================
Files 21 21
Lines 4258 4351 +93
Branches 136 137 +1
==========================================
+ Hits 4151 4243 +92
- Misses 106 107 +1
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merging this PR will improve performance by 53.66%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | parse_relative_time_complex |
160.6 µs | 62.2 µs | ×2.6 |
| ⚡ | parse_datetime_ending_in_z |
138.1 µs | 58.7 µs | ×2.4 |
| ⚡ | parse_datetime_with_tz_name |
140.5 µs | 60.6 µs | ×2.3 |
| ⚡ | parse_ctime_format |
155.9 µs | 71.3 µs | ×2.2 |
| ⚡ | parse_timezone_offset |
102.2 µs | 57.3 µs | +78.19% |
| ⚡ | parse_iso_datetime |
75.2 µs | 49.8 µs | +51.14% |
| ⚡ | parse_invalid_input |
59 µs | 39.3 µs | +50.18% |
| ⚡ | parse_weekday |
66.4 µs | 45.1 µs | +47.26% |
| ⚡ | parse_now |
43 µs | 29.2 µs | +47.11% |
| ⚡ | parse_iso_datetime_t_separator |
73.4 µs | 50.5 µs | +45.38% |
| ⚡ | parse_yesterday |
43.9 µs | 30.8 µs | +42.44% |
| ⚡ | parse_tomorrow |
42.8 µs | 30.4 µs | +40.6% |
| ⚡ | parse_date_slash_format |
37.7 µs | 26.8 µs | +40.5% |
| ⚡ | parse_datetime_with_delta |
105.6 µs | 76.1 µs | +38.67% |
| ⚡ | parse_date_only |
38.3 µs | 28 µs | +36.72% |
| ⚡ | parse_extended_large_year |
36.8 µs | 27 µs | +36.07% |
| ⚡ | parse_extended_year |
36.7 µs | 27 µs | +35.94% |
| ⚡ | parse_epoch_timestamp |
14.8 µs | 11.6 µs | +27.01% |
| ⚡ | parse_extended_year_relative |
70.6 µs | 56.6 µs | +24.88% |
| ⚡ | parse_extended_year_rollover |
96.2 µs | 81.6 µs | +17.95% |
| ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing anasbekheit:perf/parser-hot-paths (37eab19) with main (e25bf15)
|
impressive results! bravo |
| || trimmed.starts_with("p.m.") | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
a production function kept only for tests is a bit odd.
could the tests call parse_time_after_date directly instead?
| // Convert input to lowercase for case-insensitive parsing. | ||
| let lower = input.to_ascii_lowercase(); | ||
| let input = &mut lower.as_str(); | ||
| let lower: std::borrow::Cow<str> = if input.bytes().any(|b| b.is_ascii_uppercase()) { |
There was a problem hiding this comment.
could you please import Cow instead?
| "parse_item", | ||
| alt(( | ||
| combined::parse.map(Item::DateTime), | ||
| date::parse.map(Item::Date), |
There was a problem hiding this comment.
when iso1/iso2 fail above, date::parse tries them again.
is that intended?
| // The lookahead is only needed when an offset would actually parse, so | ||
| // try the (cheap) offset first and check for a relative time afterwards. | ||
| let start = input.checkpoint(); | ||
| let result = alt((timezone_offset_colon, timezone_offset_colonless)).parse_next(input); |
There was a problem hiding this comment.
please add a test for "UTC +8 years" and "+8 years" since the lookahead order changed
| @@ -215,15 +225,12 @@ fn timezone_name_offset(input: &mut &str) -> ModalResult<Offset> { | |||
| // second way, so we do the same here. | |||
| // | |||
| // Only process if the input cannot be parsed as a relative time. | |||
There was a problem hiding this comment.
same here, "Only process if..." no longer matches the code, please drop it
| { | ||
| // Fast path: most calls see no comment or ignorable sign | ||
| multispace0.parse_next(input)?; | ||
| if !input.starts_with(['(', '-', '+']) { |
There was a problem hiding this comment.
please add tests with comments and ignored signs, e.g. "1997-01-01 (foo) 10:00" and "- 1997-01-01".
i don't see any, and this fast path skips them
| @@ -198,11 +198,21 @@ pub(super) fn timezone_offset(input: &mut &str) -> ModalResult<Offset> { | |||
| // "+8 years". GNU date parses them the second way, so we do the same here. | |||
| // | |||
| // Return early if the input can be parsed as a relative time. | |||
There was a problem hiding this comment.
the old "Return early..." line is now misleading, and the comment keeps growing.
please merge them into one or two short lines :)
|
Are there any concerns blocking the merge of this PR? |
2 participants
Profiling showed most parse time went to repeated whitespace skipping and duplicate lookaheads, not date math.
"-3". The relative-time lookahead only runs when an offset actually parses.No behavior change; all existing tests pass.
1997-01-19 08:17:48 BRT1 year 3 months 2 days agoWed Jan 1 00:00:00 19972021-02-14 06:37:47 +00001997-01-01now