-
Notifications
You must be signed in to change notification settings - Fork 61
Switch to non recursive quicksort #70
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
base: main
Are you sure you want to change the base?
Conversation
|
215 to 255 is a 19% increase, which is quite significant — I'd love to see if we can fix this without a big regression for all cases. In theory, the non-recursive version shouldn't be slower — I'll see if there are any microoptimizations available here... |
|
I guess microoptimizations are possible however it being slower very much depends on the JS engine as well the hardware. A function call stack is handled differently from a custom stack. Anyhow there are also other options to get faster, i.e. by using insertion sort once the partition is small enough. |
|
@muendlein for type checking, we can add ignores similar to the existing ones in places where it's hard to type soundly: https://github.com/mourner/flatbush/blob/main/index.js#L304 |
|
@mourner I have added some performance improvements which now brings the performance to the same speed. |
Fixes #69
I have now switched to a non recursive implementation in order to solve this once and for all. There is a minor performance penalty for the indexing part (215ms -> 255ms) which I still think is worth it. Even with an optimized recursive implementation there can be datasets that fail or that require random numbers in the algorithm which would also slow it down.
@mourner What do you think?