Calendar benchmarks

[andrewpollack]

Fixes #1791

Changes:

• Benches for Calendar Date and DateTime
  • Overview focuses only on ISO, and does instantiation, arithmetic, and value retrieval across several ISO input dates
  • Drill-downs go into each calendar type, with instantiation, Conversion from ISO, Arithmetic, Retrieving vals, and Conversion to ISO.
• Name change of new_gregorian_datetime_from_integers --> new_gregorian_datetime to bring in-line with all other calendars
• Removal of nanosecond requirement on DateTime for Ethiopic/Gregorian
• Adding icu_datetime to icu_datetime [dev-dependencies]. Without this, icu_datetime tests cause compiler error delayed_good_path_bugs on my setup. This is the case even on main branch currently. See Add Date::new_from_iso(date_iso, X) example for each calendar X #1844 (comment) for same happening on icu_calendar.

[Manishearth]

Looks good so far

@zbraniecki or @sffc, want to have a second look at these?

[andrewpollack]

I'm noticing some possible bad behavior from benchmarking date.to_iso(). Filed an issue explaining at #1890

[andrewpollack]

Updated to use more generic functions where possible (thank you @Manishearth for the guidance!).

Of note, there were some cases that led to some specific cases being built, violating DRY:
Date:

• bench_calendar_iso() covers cases where IsoYear, IsoMonth, and IsoDay are required are not generically compatible. These include Date::new_iso_date and Date::new_gregorian_date.

DateTime:

• bench_calendar_nano covers cases that require nanoseconds supplied in the argument. Currently these are DateTime::new_ethiopic_datetime and DateTime::new_gregorian_datetime_from_integers. Looking now into a way to create an argument that can cover the two different versions of creation (one with, and one without nano). Covered by bench_calendar_nano

[Manishearth]

• bench_calendar_nano covers cases that require nanoseconds supplied in the argument. Currently these are DateTime::new_ethiopic_datetime and DateTime::new_gregorian_datetime_from_integers. Looking now into a way to create an argument that can cover the two different versions of creation (one with, and one without nano). Covered by bench_calendar_nano

I think we should aim for consistency across calendars here, and either everyone gets this API or nobody does. Given that DateTimes are easily mutated I somewhat prefer the latter.

[Manishearth]

Overall looks good, I think we should have the construction APIs for datetimes be largely consistent and not deal with nanoseconds, aside from that should be almost ready to merge!

[andrewpollack]

• bench_calendar_nano covers cases that require nanoseconds supplied in the argument. Currently these are DateTime::new_ethiopic_datetime and DateTime::new_gregorian_datetime_from_integers. Looking now into a way to create an argument that can cover the two different versions of creation (one with, and one without nano). Covered by bench_calendar_nano

I think we should aim for consistency across calendars here, and either everyone gets this API or nobody does. Given that DateTimes are easily mutated I somewhat prefer the latter.

What's the best way to open this discussion? Should I open an issue to decide on adding/removing nanoseconds?

[Manishearth]

What's the best way to open this discussion? Should I open an issue to decide on adding/removing nanoseconds?

Honestly I'd be fine with the change being made in this PR itself to make your benchmarks simpler (these are new APIs, it's fine) but we could do it separately if you'd like, I'd just file an issue.

[andrewpollack]

What's the best way to open this discussion? Should I open an issue to decide on adding/removing nanoseconds?

Honestly I'd be fine with the change being made in this PR itself to make your benchmarks simpler (these are new APIs, it's fine) but we could do it separately if you'd like, I'd just file an issue.

Happy to make the change to nanoseconds directly in this PR! To confirm, I will be removing all nanosecond-related references from the ethiopic and gregorian calendars.

[Manishearth]

Correct! We should construct datetimes with h/m/s and nothing more, folks can tack on ns if they really need after the fact

[andrewpollack]

Correct! We should construct datetimes with h/m/s and nothing more, folks can tack on ns if they really need after the fact

ethiopic was an easy switch, but gregorian has some additional dependencies that rely on the nanoseconds. For example https://github.com/unicode-org/icu4x/blob/main/components/datetime/src/mock/mod.rs#L39 . I'll shift the nanosecond for ethiopic, but feel less empowered to remove gregorian nanoseconds given these dependencies

[Manishearth]

I'd switch that one over too, remember that the nanoseconds field is public and you can mutate it after the fact.

But if you really want you could also make a _from_integers_with_nanos or something

[Manishearth]

This needs to have merge conflicts fixed and then can be merged

[sffc]

LGTM but I think the comment in the dependencies file is obsolete now

[Manishearth]

Great work!