iamkun / dayjs

⏰ Day.js 2kB immutable date-time library alternative to Moment.js with the same modern API
https://day.js.org
MIT License
46.9k stars 2.3k forks source link

Cache toLocaleString #1236

Open janousek opened 3 years ago

janousek commented 3 years ago

Calls to "toLocaleString" in the timezone plugin is extremly slow: https://github.com/iamkun/dayjs/blob/ed9629b5ab2895652111fc854e6081422ed5c010/src/plugin/timezone/index.js#L99

The same problem was discussed in luxon: https://github.com/moment/luxon/issues/352

The solution for this performance problem is caching like they did in luxon: https://github.com/moment/luxon/commit/bb77d5e6b968b0a2cc1be72d295f40a43a6687fb

Just use Intl.DateTimeFormat instead of "toLocaleString" (it always produce the equivalent result) and cache these instances of Intl.DateTimeFormat.

defghy commented 2 years ago

I don't know it's same problem or not. When we replace 'moment-timezone' with 'dayjs.tz', it's very slow.

Here is performance

origin_1ig9898mvwcu74a2sg9ntuhbb0orub97eiaiyud20nwdh2sjcpa origin_1k0lnmv1pvb3uw3suw6i7umzyzfvfpe33fmrtl7jhnzi0lf6d5c origin_7t19awy843emvs26lk82l5pliqk2nxbppens44r571r7zctfv origin_7tn2zs00hdkh0p5wv3pd7co78s9x0e67zneej6adwljt9i2a9

All point to proto.tz

half-shell commented 2 years ago

Hi, it seems this issue does not have any visibility. We are observing the same damming performance on our end, and it is starting to become an issue.

image

We might move away from the timezone plugin for some of our hot functions, but I'd be happy to contribute back to a package that we use so extensively.

It seems like the timezone plugin is the only place in the codebase where there's still usage of toLocaleString(). I'd be keen on submitting a PR that would implement the solution presented by @janousek , but I'm afraid I am unsure of how dayjs manages caching. If anyone can point me to some file or relevant function, I'd be happy to give it a try.

nuria commented 2 years ago

This code seems to have landed in a different form on 1.11.5? https://github.com/iamkun/dayjs/blob/dev/src/plugin/timezone/index.js#L12

Now, we continued to have issues even with that version because const target = date.toLocaleString('en-US', { timeZone: timezone })

ends up initializing a new version of Intl.DateTimeFormat each time, per MDN docs

We changed that with the following for better perf results: const target = getDateTimeFormat(timezone).format(date);

billoneil commented 2 weeks ago

@nuria how did you expose getDateTimeFormat since it isn't exported, was it just forked or copied?