-
-
Notifications
You must be signed in to change notification settings - Fork 519
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
between
is 10X slower than before when there is a TZID in the rrule.
#580
Comments
between
is 10X slower than before when there is a TZID in the rrule.
Looks like the Luxon dependency was removed in PR #508 to reduce load times in-browser. Luxon is really good at handling timezones, especially when compared to the native Date library. So that’s a bummer. |
Also noticed huge performance issues. We're seeing vast improvements by using the rrule-rust package. |
Yeah, there isn't a great native replacement for Luxon's TZ functionality. Most of the cost is in the const dateTZtoISO8601 = function (date: Date, timeZone: string) {
// date format for sv-SE is almost ISO8601
const dateStr = date.toLocaleString('sv-SE', { timeZone })
// '2023-02-07 10:41:36'
return dateStr.replace(' ', 'T') + 'Z'
}
export const dateInTimeZone = function (date: Date, timeZone: string) {
const localTimeZone = Intl.DateTimeFormat().resolvedOptions().timeZone
// Date constructor can only reliably parse dates in ISO8601 format
const dateInLocalTZ = new Date(dateTZtoISO8601(date, localTimeZone))
const dateInTargetTZ = new Date(dateTZtoISO8601(date, timeZone ?? 'UTC'))
const tzOffset = dateInTargetTZ.getTime() - dateInLocalTZ.getTime()
return new Date(date.getTime() - tzOffset)
} I see a 25% speed up by moving this line to the top-level scope: const localTimeZone = Intl.DateTimeFormat().resolvedOptions().timeZone So that's worth doing. Will have to investigate more to see what else can be sped up. |
Hello! I'm having the same problem. After doing some research, I discovered the following: Rule1 : freq=DAILY;dtstart;TZID=Europe/Helsinki:20200102T020000;interval=1;byweekday=MO,TU,WE,TH,FR,SA,SU; Although both rules provide the same dates, Rule 1 takes 681 ms locally and Rule 2 takes 0.79ms. I then discovered that the between call takes longer if the rules dtstart and betweens parameter after are far apart. I am using rule 2.8.1 with the test code below but the same problem is also in 2.7.2.
|
In our case, most of the time the local timezone is equal to the timezone in the rrule string. So we got a big performance increase after stripping any mentions of the local timezone. function removeRedundantTzId(rrule: string) {
// workaround for this bug: https://github.com/jkbrzt/rrule/issues/580
const localTzid = Intl.DateTimeFormat().resolvedOptions().timeZone;
return rrule.replace(`;TZID=${localTzid}`, "");
} edit: In the end, this workaround was not enough for us. We switched to rschedule and achieved a huge performance increase. Calculating all events in a certain duration went from ~700ms to ~20ms. |
between
is 10X slower than before when there is a TZID in the rrule.Between the version 2.6.4 and the version 2.7.0+,
between
is 10X slower if there is aTZID
in the rrule.With the following benchmark, I observe the following performance:
Even more striking, without the
TZID
, it only takes 0.7ms on average with the version 2.7.4.The text was updated successfully, but these errors were encountered: