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

[perf] isEnglish and supportsFastNumbers are slow - new Intl.DateTimeFormat #1428

Closed
MonstraG opened this issue May 1, 2023 · 3 comments · Fixed by #1642
Closed

[perf] isEnglish and supportsFastNumbers are slow - new Intl.DateTimeFormat #1428

MonstraG opened this issue May 1, 2023 · 3 comments · Fixed by #1642

Comments

@MonstraG
Copy link
Contributor

MonstraG commented May 1, 2023

I'm profiling a calendar page which shows a bunch of events. I collect a profiling file and open it in https://www.speedscope.app, look at sandwich view, and see that isEnglish is the 3th slowest function (by self time) (and supportsFastNumbers is 5th):
image

I take a look at isEnglish :

luxon/src/impl/locale.js

Lines 479 to 485 in 0702328

isEnglish() {
return (
this.locale === "en" ||
this.locale.toLowerCase() === "en-us" ||
new Intl.DateTimeFormat(this.intl).resolvedOptions().locale.startsWith("en-us")
);
}

As I'm working in Norwegian locale, I assume this returns false, but most importantly - creates new Intl.DateTimeFormat on every call.

I have not tried this yet, but I believe we can win some time if we cache Intl.DateTimeFormat (or avoid doing this check).

Same happens in supportsFastNumbers.

  • OS: Win 11
  • Browser: Chrome 112.0.5615.138
  • Luxon: 3.3.0
@MonstraG MonstraG changed the title [perf] isEnglish and supportsFastNumbers are slow [perf] isEnglish and supportsFastNumbers are slow - new Intl.DateTimeFormat May 1, 2023
@MonstraG
Copy link
Contributor Author

Looked at isEnglish a bit, and it seems that it's used to check whether to show a default built-in english locale or a proper Intl.

  1. This implementation of isEnglish fails for en-GB which is not considered english.
  2. Do we still need to ship this default locale at all?

@icambron
Copy link
Member

Yeah, isEnglish really means isUSEnglish. We use it because it's much faster than using Intl, and many pre-built formats use it (e.g. parsing RFC2822 dates). The expensive part is checking if we can use it, and I think we can probably speed that up and/or make it less likely to be needed.

@nodify-at
Copy link
Contributor

Hi team,

Is there a plan to improve this function? We've identified significant performance issues related to this function call, specifically with the line in Node.JS.

new Intl.DateTimeFormat(this.intl).resolvedOptions().locale.startsWith("en-us")

This part appears to consume a considerable amount of CPU, leading to noticeable performance
degradation.

Any insights or plans for optimization would be greatly appreciated.

Thank you!

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

Successfully merging a pull request may close this issue.

3 participants