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

Cache toLocaleString #1236

Open
janousek opened this issue Nov 22, 2020 · 4 comments · May be fixed by #2019
Open

Cache toLocaleString #1236

janousek opened this issue Nov 22, 2020 · 4 comments · May be fixed by #2019

Comments

@janousek
Copy link

Calls to "toLocaleString" in the timezone plugin is extremly slow:

const target = this.toDate().toLocaleString('en-US', { timeZone: timezone })

The same problem was discussed in luxon: moment/luxon#352

The solution for this performance problem is caching like they did in luxon: moment/luxon@bb77d5e

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

@defghy
Copy link

defghy commented Dec 21, 2021

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
Copy link

half-shell commented Apr 21, 2022

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
Copy link

nuria commented Sep 29, 2022

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants