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

1200% slower performance of next() and prev() upgrading to 4.6 from 2.3 #287

Open
hueniverse opened this issue Oct 1, 2022 · 7 comments

Comments

@hueniverse
Copy link

We recently upgraded the module from 2.3.7 to 4.6.0 and saw a huge performance degradation. When calling the next() method 10000 times, our benchmark went from 5000ms to 68000ms. Will provide more info as we dig deeper.

@harrisiirak
Copy link
Owner

harrisiirak commented Oct 2, 2022

Hi @hueniverse!

Thanks for reporting this. 12x performance degradation doesn't sound good indeed.

I tested it briefly in my local environment and saw 3x - 5x slowdown, depending of the input expression. Individual next / prev calls still takes mostly less than 1ms.

However, I tested my main hypothesis and benchmarked 2.18.0 and 3.0.0 (code diff) versions. There is a drastic performance degradation in 3.0.0, as we switched from moment to luxon. I expected luxon to be faster than moment.

luxon@v2 is also way slower than luxon@v1 - pinned 3.0.0 to use luxon@v2 and saw even worse performance. I'll investigate if there is something wrong with our luxon usage.

@hueniverse
Copy link
Author

Thanks @harrisiirak. Lmk if I can help.

@harrisiirak
Copy link
Owner

harrisiirak commented Oct 24, 2022

Just a quick status update.

I looked more in-depth at luxon's inner-workings and how it handles date-time (and timezone) calculations - I don't think there is anything that could be improved, in sense how luxon used in this library.

However, I think we can greatly reduce actual calls we're making to calculate/create new date objects during the iteration process. When the date is iterated, all the calculations happens in 1-step (second, minute, hour etc) interval increments/decrements. This is lazy approach and creates lots of unnecessary (which is bad for the performance) date related operations. The solution could be jump over all the date component values that we can't match anyway. I've created local POC but as expected, some of the tests are now failing - I'll look into this when I've a bit more time (and brainpower).

@threes-was-taken
Copy link

@harrisiirak Do you have a status update on this issue?

@harrisiirak
Copy link
Owner

Hi @threes-was-taken!

Sadly, I haven't had time to look into this issue further.

@threes-was-taken
Copy link

@harrisiirak
Hi! Thanks for the quick response!
No problem at all, was just wondering since we're facing these issues as well :)

@antam-dayat
Copy link

hi! sorry, any updates for this issue?

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

No branches or pull requests

4 participants