dayjs icon indicating copy to clipboard operation
dayjs copied to clipboard

Fix: performance issue in tz plugin

Open logeyG opened this issue 3 years ago • 21 comments
trafficstars

This PR is an adaptation of the following closed PR: https://github.com/iamkun/dayjs/pull/1889

Fixes performance issue in timezone plugin by using a cached Intl.DateTimeFormat formatter instead of toLocaleString(). No functional changes, just an order of magnitudes faster.

The only change I made was to create a default config object that is used for both getDateTimeFormat as well as getLocaleStringifier.

Fixes https://github.com/iamkun/dayjs/issues/1236

logeyG avatar Aug 08 '22 13:08 logeyG

image It seems this 2 function not generate same format;

In my case, getLocaleStringifier worked fine, use getDateTimeFormat make date not correct

defghy avatar Aug 10 '22 07:08 defghy

Hi @defghy, yes you are correct, but I don't think this is an issue. The dates are the same, just in different formats. The previous code that getLocaleStringifier is replacing had the exact same format when using toLocaleString instead (see line 115):

let t = new Date().toLocaleString('en-US', { timeZone: 'Europe/Paris' }) '8/10/2022, 10:07:10 AM'

logeyG avatar Aug 10 '22 08:08 logeyG

Has some problem when date is Invalid Date;

image

In this situation, an Error throw; Maybe need some try/catch to remain same as before

@logeyG

defghy avatar Aug 11 '22 09:08 defghy

@defghy this actually seems to be an improvement, in my opinion.

I think it is OK to throw an error if anything other than a date object is passed in. I can't think of an instance where catching the error is the preferred behavior here.

logeyG avatar Aug 12 '22 08:08 logeyG

@iamkun can you please review this?

logeyG avatar Aug 23 '22 08:08 logeyG

@iamkun can you please review this?

logeyG avatar Sep 19 '22 08:09 logeyG

@BePo65 @iamkun any chance of getting this reviewed?

Prabhakar-Poudel avatar Sep 23 '22 12:09 Prabhakar-Poudel

Anyone know when this could be merged please? We are blocked from using DayJS due to the performance issue mentioned here. @logeyG @iamkun

kamrankhan54 avatar Jan 23 '23 15:01 kamrankhan54

up 👍

Klaitos avatar Mar 16 '23 09:03 Klaitos

up up up)

Serg-Mois avatar Mar 16 '23 09:03 Serg-Mois

up up ?

tgensol avatar Apr 06 '23 16:04 tgensol

Why haven't there been any updates for this issue?

tduyng avatar Apr 14 '23 09:04 tduyng

Repo looks like dead, the only author doesn't support it.

serg-mois-capital avatar Apr 14 '23 09:04 serg-mois-capital

up

dominikbrazdil avatar Apr 21 '23 06:04 dominikbrazdil

Thanks for making such an awesome project @logeyG @iamkun ! I'm hoping to bump this up in your priority list to review as the current state is currently contributing to 2.5% of my nodejs backend application's CPU profile with a wall time of about 162ms. This would be an easy win for improving our applications speed. We've seen tons of performance improvements since switching from moment but this one is definitely an outlier.

Thanks again!

kozak-codes avatar May 29 '23 22:05 kozak-codes

@logeyG This fix is not correct. Try this:

let date = new Date(2023,9,2,0,0,0);

let val1 = new Intl.DateTimeFormat('en-US', {
		year: 'numeric',
		month: '2-digit',
		day: '2-digit',
		hour: '2-digit',
		minute: '2-digit',
		second: '2-digit',
		hour12: false,
		timeZone: 'Europe/Prague',
    })
	.format(date);

let val2 = date.toLocaleString('en-US', { timeZone: 'Europe/Prague' });


console.log(val1, 'VS', val2);
console.log(new Date(val1), 'VS', new Date(val2));

image

I belive that you have to set hour12 to true.

janousek avatar Aug 02 '23 11:08 janousek

@iamkun Bumping this as well. Seeing major performance issues with using the tz plugin as it exists now. The changes proposed in this PR result in orders of magnitude better perf. Happy to help with testing or whatever is needed. Let me know. 🙏

keithwillcode avatar Feb 02 '24 15:02 keithwillcode

Hello guys, firstly I'd like to thank you @iamkun for all for the effort you put into enhancing the dayjs library.

I've also encountered some performance challenges related to the timezone function, which have been impacting my work. I came across PR #2542 and noticed that it addresses these issues effectively, as well as this PR.

Could you share any insights on the timeline for the next release that would include these significant improvements? This update would greatly benefit our projects, and we're looking forward to integrating it.

Thank you for your continued dedication to improving dayjs, we love it.

JorgeHabib avatar Feb 15 '24 20:02 JorgeHabib

For temporary measure, you guys can use my forked library for the timezone issue. I have merged this PR.

https://www.npmjs.com/package/@adlanarifzr/dayjs

import timezone from '@adlanarifzr/dayjs/plugin/timezone';
dayjs.extend(timezone);

adlanarifzr avatar Feb 16 '24 00:02 adlanarifzr

For temporary measure, you guys can use my forked library for the timezone issue. I have merged this PR.

https://www.npmjs.com/package/@adlanarifzr/dayjs

import timezone from '@adlanarifzr/dayjs/plugin/timezone';
dayjs.extend(timezone);

These improvements have significantly enhanced our performance here. Are you/anyone aware of any other functions that might be encountering similar performance issues?

JorgeHabib avatar Feb 18 '24 19:02 JorgeHabib

@iamkun Is it possible for you to review this? It solves a major performance issue with the timezone plugin

rushi avatar Apr 28 '24 08:04 rushi