Optimize Performance with Caching for Resolved Options and Parsed Locale Strings #1642

nodify-at · 2024-06-27T13:11:26Z

Changes

Caching: Implemented caching to avoid redundant computations of resolved options and locale parsing.
Efficient Checks: Introduced a final isEnglish variable to efficiently determine if the locale supports English.
Initialization: supportsFastNumbers is now immediately initialized to prevent unnecessary calls.

Performance Impact

Tests indicate up to a 10x speed improvement with these enhancements.

Issue Fixed

… parsed locale strings - Implement caching to avoid redundant computations of resolved options and locale parsing. - Introduce a final `isEnglish` variable to efficiently check if the locale supports English. Signed-off-by: nodify-at <21654050+nodify-at@users.noreply.github.com>

linux-foundation-easycla · 2024-06-27T13:11:30Z

The committers listed above are authorized under a signed CLA.

✅ login: nodify-at / name: Hasan Ozdemir (a2f6f69, 9e2011b, 05c075b, 456325e)

nodify-at · 2024-06-28T18:52:15Z

Is there any update on this or can someone give me a feedback if I've to improve this PR? :)

flodlc · 2024-07-05T22:49:50Z

Once this one is merged, could we expect a new release ? There has been many perf improvements that would benefits to the community.

icambron · 2024-07-08T15:24:06Z

I'll take a look at this one soon

nodify-at · 2024-07-08T15:57:04Z

Thanks @icambron :)

Seledrex · 2024-07-19T14:21:47Z

Nice work! I was just investigating this problem myself and stumbled upon this issue. Definitely looking forward to this performance improvement! 🚀

nodify-at · 2024-07-22T14:12:45Z

Hi @icambron,

When would you have time to review this? We're using a temporary patch right now, and it would be great to have this feature in Luxon. This will help us remove the workaround and benefit the community, especially for Node.js apps.

Thanks!

icambron · 2024-08-03T21:11:32Z

src/impl/locale.js

@@ -374,14 +394,14 @@ export default class Locale {
    this.eraCache = {};

    this.specifiedLocale = specifiedLocale;
-    this.fastNumbersCached = null;
+    this.fastNumbersCached = supportsFastNumbers(this);


I guess I don't quite know what moving this here is fixing. We may never use fastNumbers(), so I don't love the idea of computing it upfront instead of computing it lazily and only caching it then.

icambron · 2024-08-03T21:12:12Z

src/impl/locale.js

@@ -374,14 +394,14 @@ export default class Locale {
    this.eraCache = {};

    this.specifiedLocale = specifiedLocale;
-    this.fastNumbersCached = null;
+    this.fastNumbersCached = supportsFastNumbers(this);
+    this.isEnglishCached =


Similarly, I agree we should cache this, but why not cache it inside isEnglish? That way if isEnglish is never called, we don't pay the performance penalty

icambron · 2024-08-03T21:14:04Z

src/impl/locale.js

  }
+  intlConfigCache[key] = localeStr;


I'm having a hard time understanding how this would give such a big speedup. Can you use the benchmarks in the repo to demonstrate this?

icambron · 2024-08-03T21:16:30Z

src/impl/locale.js

@@ -73,7 +81,18 @@ function getCachedWeekInfo(locString) {
  return data;
 }

-function parseLocaleString(localeStr) {
+const localeStringCache = {};
+function getParsedCachedLocaleString(localeStr) {


I can believe that this parsing is slow, but I'd like to see the benchmarks on this too

icambron

Sorry for taking so long to review this. I left some comments

…ers and isEnglish calls to improve performance Signed-off-by: nodify-at <21654050+nodify-at@users.noreply.github.com>

nodify-at · 2024-08-04T15:26:07Z

@icambron Thanks for your feedback. I took a thorough approach initially to improve the calls, but I've now simplified it to focus on the isEnglish and fastNumbers calls only. I've also cleaned up all other improvements/changes. Could you please review it again?

… Intl.resolvedOptions cache Signed-off-by: nodify-at <21654050+nodify-at@users.noreply.github.com>

nodify-at · 2024-08-04T19:43:32Z

@icambron
I hope I did this correctly. I also added two new benchmark tests to compare the results.

Here are the initial results from my local environment:

DateTime#format in German: 301,824 ops/sec ±0.41% (95 runs sampled)
DateTime#format in German with no-cache: 11,999 ops/sec ±3.03% (88 runs sampled)

nodify-at · 2024-08-10T15:32:38Z

@icambron did you get my last changes ?

flodlc · 2024-08-27T08:11:31Z

I'm starting to lose hope that this PR will ever get merged 😞

Seledrex · 2024-08-27T10:33:16Z

I'm starting to lose hope that this PR will ever get merged 😞

Just give it some more time!

icambron · 2024-09-04T01:57:25Z

Looks good, merged. Thanks for the contribution. I will release this soon.

nodify-at · 2024-09-04T09:51:59Z

Awesome :) Thanks @icambron

mohd-akram · 2024-09-11T09:27:02Z

src/impl/locale.js

@@ -167,7 +175,7 @@ function supportsFastNumbers(loc) {
      loc.numberingSystem === "latn" ||
      !loc.locale ||
      loc.locale.startsWith("en") ||
-      new Intl.DateTimeFormat(loc.intl).resolvedOptions().numberingSystem === "latn"
+      getCachedIntResolvedOptions(loc.locale).numberingSystem === "latn"


Should this be loc.intl?

It looks a bit strange, because in my last commit I reverted this change. getCachedIntResolvedOptions function is used only in supportsFastNumbers and isEnglish .

See the code in master: https://github.com/moment/luxon/blob/master/src/impl/locale.js

Ops, I see your pointed code better now, sorry and you're right it should be intl.

nodify-at changed the title ~~Improve Performance by caching resolved options for isEnglish and supportsFastNumbers~~ Optimize Performance with Caching for Resolved Options and Parsed Locale Strings Jun 27, 2024

icambron force-pushed the master branch from b54eeef to a4044fe Compare August 3, 2024 20:47

icambron reviewed Aug 3, 2024

View reviewed changes

icambron requested changes Aug 3, 2024

View reviewed changes

feat: simplify caching logic for resolved options in supportsFastNumb…

a2f6f69

…ers and isEnglish calls to improve performance Signed-off-by: nodify-at <21654050+nodify-at@users.noreply.github.com>

feat: add new benchmark tests to compare performance with and without…

456325e

… Intl.resolvedOptions cache Signed-off-by: nodify-at <21654050+nodify-at@users.noreply.github.com>

Merge branch 'moment:master' into master

05c075b

icambron approved these changes Sep 4, 2024

View reviewed changes

icambron merged commit 3e9983c into moment:master Sep 4, 2024
1 check passed

mohd-akram reviewed Sep 11, 2024

View reviewed changes

This was referenced Sep 11, 2024

Fixed Caching for Resolved Options #1653

Closed

Fixed parameter to check cached resolved options #1654

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize Performance with Caching for Resolved Options and Parsed Locale Strings #1642

Optimize Performance with Caching for Resolved Options and Parsed Locale Strings #1642

nodify-at commented Jun 27, 2024 •

edited

Loading

linux-foundation-easycla bot commented Jun 27, 2024 •

edited

Loading

nodify-at commented Jun 28, 2024

flodlc commented Jul 5, 2024

icambron commented Jul 8, 2024

nodify-at commented Jul 8, 2024

Seledrex commented Jul 19, 2024

nodify-at commented Jul 22, 2024

icambron Aug 3, 2024 •

edited

Loading

icambron Aug 3, 2024 •

edited

Loading

icambron Aug 3, 2024

icambron Aug 3, 2024

icambron left a comment

nodify-at commented Aug 4, 2024

nodify-at commented Aug 4, 2024

nodify-at commented Aug 10, 2024

flodlc commented Aug 27, 2024

Seledrex commented Aug 27, 2024

icambron commented Sep 4, 2024

nodify-at commented Sep 4, 2024

mohd-akram Sep 11, 2024

nodify-at Sep 11, 2024

nodify-at Sep 11, 2024

nodify-at Sep 11, 2024

Optimize Performance with Caching for Resolved Options and Parsed Locale Strings #1642

Optimize Performance with Caching for Resolved Options and Parsed Locale Strings #1642

Conversation

nodify-at commented Jun 27, 2024 • edited Loading

Changes

Performance Impact

Issue Fixed

linux-foundation-easycla bot commented Jun 27, 2024 • edited Loading

nodify-at commented Jun 28, 2024

flodlc commented Jul 5, 2024

icambron commented Jul 8, 2024

nodify-at commented Jul 8, 2024

Seledrex commented Jul 19, 2024

nodify-at commented Jul 22, 2024

icambron Aug 3, 2024 • edited Loading

Choose a reason for hiding this comment

icambron Aug 3, 2024 • edited Loading

Choose a reason for hiding this comment

icambron Aug 3, 2024

Choose a reason for hiding this comment

icambron Aug 3, 2024

Choose a reason for hiding this comment

icambron left a comment

Choose a reason for hiding this comment

nodify-at commented Aug 4, 2024

nodify-at commented Aug 4, 2024

nodify-at commented Aug 10, 2024

flodlc commented Aug 27, 2024

Seledrex commented Aug 27, 2024

icambron commented Sep 4, 2024

nodify-at commented Sep 4, 2024

mohd-akram Sep 11, 2024

Choose a reason for hiding this comment

nodify-at Sep 11, 2024

Choose a reason for hiding this comment

nodify-at Sep 11, 2024

Choose a reason for hiding this comment

nodify-at Sep 11, 2024

Choose a reason for hiding this comment

nodify-at commented Jun 27, 2024 •

edited

Loading

linux-foundation-easycla bot commented Jun 27, 2024 •

edited

Loading

icambron Aug 3, 2024 •

edited

Loading

icambron Aug 3, 2024 •

edited

Loading