perf(@formatjs/ecma402-abstract): cache NumberFormat instances for 20x speed-up when formatting dates

[andreialecu]

Fixes #2807
Fixes #2813

It appears that creating new Intl.NumberFormat instances is extremely slow. Caching them by locale key seems safe, and this dramatically increases performance.

Before:

format (polyfill) x 15,193 ops/sec ±2.86% (87 runs sampled)
format (native) x 551,023 ops/sec ±4.01% (83 runs sampled)

After:

format (polyfill) x 326,105 ops/sec ±0.42% (98 runs sampled)
format (native) x 643,658 ops/sec ±0.62% (93 runs sampled)

cc @longlho

[longlho]

This should be resolved with caching the constructor upstream right?

[andreialecu]

Not sure what you mean by caching the constructor upstream.

I'm using the benchmark that is part of the repo, which caches the DateTimeFormat constructor.

[longlho]

As in user should cache new Intl.NumberFormat regardless of if it's polyfills or native

[andreialecu]

I'm still confused by what you mean. This isn't about caching Intl.NumberFormat, the function I modified creates new instances of Intl.NumberFormat as part of formatting a date.

Feel free to review it when time permits.

[longlho]

Oh sorry I misread the filename. As I mentioned this behavior is not spec compliant so we won't merge it.

[andreialecu]

Just an opinion, but I think this repository and everyone using and contributing to it would greatly benefit if the handling of issues was taken more seriously.

I'd have appreciated not spending so much time in #2807 and #2813 testing and replying to one-liners just to re-explain something that could be understood just by treating things seriously. Thankfully the issue was finally acknowledged and some steps started to be taken in: 812bcb6

#2807 in particular was closed and locked with a comment in which the first paragraph was proven incorrect by: 463420c, while other comments like facebook/hermes#23 (comment) are simply not backed by any real numbers.

I even read the spec, and don't see how this could cause any problems, considering the changes simply add a private cache that is only used in one specific place.

My final hope was that, after kindly asking, you'd spend some time to review a 20x performance increase that actually unblocks a lot of people and stops wasting a lot of resources. I also noticed a dramatic reduction in GCs by using this patch.

I'll deal with using a patch via patch-package, but this would've been useful to the people in facebook/hermes#23 as well.

[longlho]

I'd suggest discussing your direction before spending time on implementation. As I mentioned we try to keep it as close to the spec as possible and slipping in 463420c is already breaking that but it's small enough I'm ok with it. Introducing new caching is a much bigger change:

1. The behavior is not defined in the spec.
2. It creates a side effect which is a global cache with no ability to flush.
3. Underlying data can change, it's not guaranteed to be static by the cache. Even in this polyfill you can add/invalidate data at runtime.

Basically my rule of thumb is, if you're changing the polyfill, please link to the line either in the spec, or in reference implementations such as icu4j/icu4c.

[andreialecu]

The time spent implementing was useful to me. This patch is actually something I need and is the only way to even make this feasible to use. So I'm good on that.

The only time I feel is wasted is the time I spent trying to communicate about this issue.

Regarding caching, from what I can see in the spec https://402.ecma-international.org/7.0/#sec-partitiondatetimepattern:

Let locale be dateTimeFormat.[[Locale]].
Let nfOptions be ObjectCreate(null).
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 »).

The nfOptions to construct nf and nf2 are static and they never change. The only thing that varies is the locale.

If users add or remove locale data at runtime, then that can be a problem, but that seems to go against the spec as well. I wasn't able to find anything in the spec that mentions this being a supported scenario.

Even so, a function could be exported to allow clearing the cache. Or even have the cache disabled by default, and require manually enabling it somehow.

[longlho]

If users add or remove locale data at runtime, then that can be a problem, but that seems to go against the spec as well.

The spec is intentionally being vague about data source/shape as we don't want to tie it to a specific repo like CLDR.