Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize NaiveDate::add_days for small values #1214

Merged
merged 2 commits into from
Sep 12, 2023

Conversation

pitdicker
Copy link
Collaborator

I expect this method to be used much more often with small values than with values that end up an x number of years into the past or future. So I would like to add a fast path.

In the common case where the result of add_checked_signed and similar ends up within the same year we can avoid a couple of divisions and lookups and just change the ordinal.

Before:

bench_naivedate_add_signed
                        time:   [32.797 ns 34.745 ns 36.560 ns]
Found 13 outliers among 100 measurements (13.00%)
  13 (13.00%) high severe

After:

bench_naivedate_add_signed
                        time:   [12.886 ns 13.422 ns 13.855 ns]
                        change: [-65.054% -63.390% -61.655%] (p = 0.00 < 0.05)
                        Performance has improved.

src/naive/date.rs Show resolved Hide resolved
src/naive/date.rs Show resolved Hide resolved
@pitdicker
Copy link
Collaborator Author

Does this have good test coverage?

I'll have a look later today

That must have been one of the longer days in history...

With a small change in add_days to make the result invalid we have 37 failing tests:

failures:
    datetime::tests::test_add_sub_months
    datetime::tests::test_datetime_add_assign_local
    datetime::tests::test_datetime_add_days
    datetime::tests::test_datetime_add_months
    datetime::tests::test_datetime_date_and_time
    datetime::tests::test_datetime_fixed_offset
    datetime::tests::test_datetime_from_local
    datetime::tests::test_datetime_offset
    datetime::tests::test_datetime_rfc2822
    datetime::tests::test_datetime_rfc3339
    datetime::tests::test_datetime_sub_assign_local
    datetime::tests::test_datetime_sub_days
    datetime::tests::test_datetime_sub_months
    datetime::tests::test_from_system_time
    datetime::tests::test_parse_datetime_utc
    datetime::tests::test_rfc3339_opts
    datetime::tests::test_utc_datetime_from_str
    format::formatting::tests::test_datetime_format_alignment
    format::parse::tests::test_rfc2822
    format::parse::tests::test_rfc3339
    format::parsed::tests::issue_551
    format::parsed::tests::test_parsed_to_naive_date
    format::parsed::tests::test_parsed_to_naive_datetime_with_offset
    month::tests::test_month_enum_primitive_parse
    month::tests::test_month_enum_try_from
    naive::date::tests::test_date_add
    naive::date::tests::test_date_add_days
    naive::date::tests::test_date_parse_from_str
    naive::date::tests::test_date_sub_days
    naive::date::tests::test_naiveweek
    naive::date::tests::test_week_iterator_limit
    naive::date::tests::test_weeks_from
    naive::datetime::tests::test_and_local_timezone
    naive::datetime::tests::test_and_utc
    naive::datetime::tests::test_datetime_add
    offset::fixed::tests::test_date_extreme_offset
    offset::local::tests::verify_correct_offsets_distant_past

More seriously: we have reasonable but not exhaustive coverage for values within a year, for values that cross the year boundary and have to account for leap years, and for values than can cause an overflow in naive::date::test_date_add_days.

Adding more tests does not seem that useful to me.

@jtmoon79
Copy link
Contributor

jtmoon79 commented Sep 2, 2023

I recommend adding a #[cfg(test)]-only code path within the add_days to turn on and turn off the fast path.
...
I recommend test cases for days values
0 to 367 inclusive that compares add_days when ADD_DAYS_ENABLE_FAST_PATH is true and false

Adding more tests does not seem that useful to me.

I think the confidence this provides is worth the relatively small amount of work. (admittedly, I'm not doing the work! though if you want me to then I can submit a PR for my suggestion)

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Merging #1214 (28c85aa) into 0.4.x (2960f2c) will increase coverage by 0.00%.
Report is 5 commits behind head on 0.4.x.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##            0.4.x    #1214   +/-   ##
=======================================
  Coverage   91.40%   91.41%           
=======================================
  Files          38       38           
  Lines       16932    16959   +27     
=======================================
+ Hits        15477    15503   +26     
- Misses       1455     1456    +1     
Files Changed Coverage Δ
src/naive/date.rs 96.30% <100.00%> (-0.05%) ⬇️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pitdicker
Copy link
Collaborator Author

@djc Did you want to give this a second look? Or was #1214 (comment) only a question and is this approved?

@pitdicker pitdicker merged commit 36ec37b into chronotope:0.4.x Sep 12, 2023
37 checks passed
@pitdicker pitdicker deleted the add_days_optim branch September 12, 2023 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants