Fix: performance issue in tz plugin. #1889
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1889 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 181 181
Lines 2064 2072 +8
Branches 537 538 +1
=========================================
+ Hits 2064 2072 +8
Continue to review full report at Codecov.
|
|
|
||
| const newLocaleStringifier = new Intl.DateTimeFormat('en-US', { | ||
| timeZone: timezone, | ||
| day: 'numeric', |
There was a problem hiding this comment.
should we leave these configs to default?
There was a problem hiding this comment.
I simply configured it with trial-and-error until it 1) worked and 2) produced the same results (identical localized string that is then used by the rest of the .tz func) that the old code did while doing some 10000 iterations on random dates and benchmarking the performance. I lack expertise and context here to judge this any further.
There was a problem hiding this comment.
Thanks, if so, share the same logic with getDateTimeFormat ?
There was a problem hiding this comment.
I of course tried reusing the getDateTimeFormat() as-is before trying anything else, but everything crashed and burned. Then I thought of reusing some of the same logic and refactoring the caching key there to work with timezone, format options and options, but decided against that because it would just have made it more complex vs making a completely independent function.
But if you'd prefer to have it that way, I can do that!
There was a problem hiding this comment.
I'm not 100% sure whether my message is easily understood, so I will try to be clearer.
It was meant to be a question. Would you prefer me to refactor this to share logic between the two caching functions? I do not think that it will make the code clearer or much shorter in this case, so I will sit on this until further notice.
There was a problem hiding this comment.
Hi. I'm not too familiar with Github so please forgive me if this is not the best place to post this.
I'm using dayjs for a website and I've been making some changes to it recently, specifically adding timezones to some functions. With the latest addition, I've noticed a drastic performance issue.
I came across this thread, but I realized this solution hasn't been applied to the production code I have in my node_modules/dayjs/plugin/timezone.js file (version 1.11.4). So I decided to give it a try on my own:
Simply adding the getLocaleStringifier function, as is, on top of the code and editing "const target" to use it, I noticed a huge performance gain of about 10 times. I use dayjs.tz() several times in a loop, in nested functions and within some ES6 array methods, so my code may not be very efficient. Regardless, this loop execution time went down from more than 5 seconds to less than 600 ms.
I hope this helps and this improvement reaches the live dayjs version soon. Find also attached two screenshots from before and after the change.
There was a problem hiding this comment.
In my case, getLocaleStringifier worked fine, use getDateTimeFormat make date not correct
|
Hi all, can I ask why this PR was closed? We ran into a similar issue with using .tz() in a nested loop. It seems as though the solution is straightforward. I'm happy to own this and open up another PR if necessary. |
I closed it after a couple months or so when it did not move and the whole issue became meaningless via the issue not moving and me coming to a conclusion that it is wisest to seek out an alternative solution and I don't have the extra energy to keep following this. I think the code itself is fine and solves the issue, though, as far as I trust my limited sense of context within the greater dayjs library. Feel free to grab it and make a new PR! |
Fixes performance issue in timezone plugin by using a cached
Intl.DateTimeFormatformatter instead oftoLocaleString(). No functional changes, just an order of magnitudes faster.Fixes #1236 .