Should we be caching the ICU4X dates?

[Manishearth]

Almost any Temporal object will contain an ISODate, and when you access it for calendared information, it will construct a Date<AnyCalendar> on the fly via Calendar::from_iso().

This seems inefficient: 99% of the time we have constructed a date we already have the from-ISO value; and even if we don't, most useful methods on Date need to call this. In some cases we can rely on the optimizer to inline and collapse consecutive from_iso() calls, but that won't work e.g. over FFI.

We should benchmark some of this, too.

Useful workloads:

• Constructing a date (via various methods)
• Calling .to_ixdtf_string()
• Calling a single calendared getter on a date
• Calling most calendared getters on a date, e.g. for formatting

[sffc]

We've designed ICU4X to optimize for from_iso being an efficient operation. The only inefficiency left are the far-away Chinese and Dangi dates, but we have a path forward to resolve those (unicode-org/icu4x#5778). We also have a path forward to improve fixed-to-iso which is another operation that will be in the fast path (Neri), which I think this project already implements.

[nekevss]

Continuously constructing ICU4X dates has always been a concern, so I'd definitely be intrigued about caching them. We should also definitely start setting up some benchmarks for this and for Duration.

[Manishearth]

@sffc Yeah, though I do wonder if there's still a cost of from_iso being called 10-12 times when it comes to formatting.

Also worried about timezone lookup costs for ZDT in that case.

Should be fine, we should perhaps benchmark it a bit and monitor.

[sffc]

@sffc Yeah, though I do wonder if there's still a cost of from_iso being called 10-12 times when it comes to formatting.

Context? That shouldn't need to happen; it's not observable

[Manishearth]

Ah, sorry, should have been clearer. Currently formatting goes through get_epoch_ms(), as requested by the spec, but if/when ICU4X and ICU4C have divergences¹ we'll need to shuttle across all the fields, which involves calling all of the .year() etc methods in sequence. That won't inline across FFI.

In theory we can create a FormattableDate struct over FFI and hope that CSE optimizations batch up the from_iso.

So there are other options for us here.

Footnotes

1. And they likely will once we do proleptic approximations! They already diverge for rgsa. ↩

[sffc]

An ffi::Date wraps a Date<AnyCalendar>, so when formatting, you should be able to convert your epoch time* to a calendar date once, and then read all the fields.

* It shouldn't be observable that the date goes through epoch time. That's just a technique the spec authors used in order to flatten all the Temporal types to a single type, and they picked epoch time since Intl.DateTimeFormat was already wired up to use epoch time.

[Manishearth]

@sffc what's ffi::Date? Are you talking about ICU4X Dates? I don't think our plan for handling ICU4X/ICU4C version skew currently involves pulling in ICU4X FFI (since we have other diplomat version problems to solve there). The hope was to reuse Temporal types.

[sffc]

OK, sure, in the temporal_rs FFI, you would either need to return an ICU4X ffi::Date or else return your own little wrapper type thing for formatting, which now I see is what you were probably thinking when you said FormattableDate.

[Manishearth]

Actually, I think this matters more for ZonedDateTime.

ZonedDateTime stores a UTC timestamp (as requested by the spec), and unconditionally calls self.tz.get_iso_datetime_for(&self.instant, provider) for most operations. Those are timezone computations that can be somewhat expensive depending on the backend.

We should either be caching a UtcOffset, or caching a full IsoDateTime.

[Manishearth]

The added benefit is that ZDT no longer needs a provider for all of its non-constructing methods. Yay!

[sffc]

You should definitely store at least a UtcOffset in ZonedDateTime.

[Manishearth]

We cached offsets in #510

I think the general consensus here is to not cache dates for now. Can reopen if it's a problem.