boa-dev
Should we be caching the ICU4X dates?
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
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 (https://github.com/unicode-org/icu4x/issues/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.
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.
@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 Yeah, though I do wonder if there's still a cost of
from_isobeing called 10-12 times when it comes to formatting.
Context? That shouldn't need to happen; it's not observable
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[^1] 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.
[^1]: And they likely will once we do proleptic approximations! They already diverge for rgsa.
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.
@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.
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.
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.
The added benefit is that ZDT no longer needs a provider for all of its non-constructing methods. Yay!
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.