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

Clarification on PartitionDateTimePattern: Constructing NumberFormat instances #563

Open
andreialecu opened this issue Apr 13, 2021 · 7 comments
Labels
c: spec Component: spec editorial issues s: help wanted Status: help wanted; needs proposal champion
Milestone

Comments

@andreialecu
Copy link

Ref: https://402.ecma-international.org/7.0/#sec-partitiondatetimepattern

Perform ! CreateDataPropertyOrThrow(nfOptions, "useGrouping", false).
Let nf be ? Construct(%NumberFormat%, « locale, nfOptions »).
Let nf2Options be ObjectCreate(null).
Perform ! CreateDataPropertyOrThrow(nf2Options, "minimumIntegerDigits", 2).
Perform ! CreateDataPropertyOrThrow(nf2Options, "useGrouping", false).
Let nf2 be ? Construct(%NumberFormat%, « locale, nf2Options »).
Let tm be ToLocalTime(x, dateTimeFormat.[[Calendar]], dateTimeFormat.[[TimeZone]]).

The steps in PartitionDateTimePattern include constructing two separate NumberFormat instances. (step 6 and 10)

This operation is called when formatting every single date and it can be really slow.

Caching the two NumberFormat instances by locale instead of constructing them every time can increase the performance of formatting dates anywhere from 20x to 100x. This is especially important for lower end devices, or for jitless runtimes.

Is there any recommendation on how to handle this? Is caching appropriate here?

How do other implementations handle this?

@sffc
Copy link
Contributor

sffc commented Apr 13, 2021

Good question. I don't see why the NumberFormat objects need to be created in the format function; it seems like they should be created in the constructor.

Note that the spec only illustrates intended observable behavior; implementations can make optimizations as long as the observed behavior is the same. So, there's nothing right now preventing an implementation from caching the NumberFormat objects.

@sffc sffc added c: spec Component: spec editorial issues s: discuss Status: TG2 must discuss to move forward labels Apr 13, 2021
@longlho
Copy link
Collaborator

longlho commented Apr 13, 2021

I think moving NumberFormat construct to the constructor would also solve the caching issue, since you can just cache DateTimeFormat constructor and that'd subsequently cache all its internal constructors like PluralRules as well.

@justingrant
Copy link
Contributor

I'm not sure if this issue describes the same underlying problem, but when I was building a proof-of-concept implementation of non-ISO calendars for Temporal, I was surprised at how slow it was to format calendar-localized dates. I'd assumed that this was inherent inefficiency in the ICU implementation, but if there's another, easier-to-resolve cause, that would be great news!

@longlho
Copy link
Collaborator

longlho commented Apr 14, 2021

@sffc proposal or PR?

@sffc
Copy link
Contributor

sffc commented Apr 23, 2021

This is editorial, so a PR would suffice.

@anba
Copy link
Contributor

anba commented Apr 26, 2021

I'd assumed that this was inherent inefficiency in the ICU implementation, but if there's another, easier-to-resolve cause, that would be great news!

ICU-based implementations only implement the first two steps of FormatDateTimePattern. Everything else is handled internally in ICU, that means the NumberFormat allocations in FormatDateTimePattern never actually happen. So this change makes zero difference in performance for JSC/V8/SM/Graal etc.


I don't see why the NumberFormat objects need to be created in the format function; it seems like they should be created in the constructor.

They were probably created there because they aren't used anywhere else, so it could be argued it's cleaner to keep these as locals within FormatDateTimePattern. But the spec doesn't use this approach consistently, for example InitializeRelativeTimeFormat creates internal NumberFormat and PluralRules objects even though they're only used within PartitionRelativeTimePattern.

@sffc sffc added this to Other Issues in ECMA-402 Meeting Topics Jan 12, 2023
@sffc sffc added s: help wanted Status: help wanted; needs proposal champion and removed s: discuss Status: TG2 must discuss to move forward labels Mar 28, 2024
@sffc sffc added this to the ES 2024 milestone Mar 28, 2024
@sffc sffc moved this from Other Issues to Previously Discussed in ECMA-402 Meeting Topics Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: spec Component: spec editorial issues s: help wanted Status: help wanted; needs proposal champion
Projects
ECMA-402 Meeting Topics
Previously Discussed
Development

Successfully merging a pull request may close this issue.

5 participants