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

feat: factorial for HUGE numbers #1691

Merged
merged 15 commits into from Dec 31, 2019
Merged

feat: factorial for HUGE numbers #1691

merged 15 commits into from Dec 31, 2019

Conversation

bartekleon
Copy link
Contributor

No description provided.

@bartekleon
Copy link
Contributor Author

@josdejong This one is faster than the previous one for numbers 1400+ (At least for BigInt) and can be up to two times faster 🚀

@bartekleon
Copy link
Contributor Author

bartekleon commented Dec 15, 2019

image

Results for the last update

@josdejong
Copy link
Owner

For reference, see #1687

@bartekleon
Copy link
Contributor Author

bartekleon commented Dec 17, 2019

@josdejong It has been finished now. I have found the crossing point, and it's surprisingly small (~30 [for BigInt it was 1400]). I have kept both algorithms in use since the fastest one is fast only for big numbers (it is quite slow for smaller numbers [slower even than normal factorial]), so now betterFactorial should be much faster than current bigFactorial for all cases (up to 3 times faster [for n = 10000 - I haven't checked for more])

IMPORTANT NOTE: bigFactorial(n) != betterFactorial(n) due to precision and toExpPos settings. These should be increased in order to get accurate (correct) value. [especially for bigger numbers]

Copy link
Collaborator

@harrysarson harrysarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just one bit of code I have a couple of thoughts about :)

test/benchmark/factorial.js Outdated Show resolved Hide resolved
@bartekleon
Copy link
Contributor Author

bartekleon commented Dec 18, 2019

@harrysarson done :)

I was too tired I think to let that Math.floor be there :P Now it should all be alrighty (also checked values. They are still the same)

@josdejong
Copy link
Owner

Thanks Bartosz, interesting improvement.

I see the performance improvement is not very large over the previous version though. I'm not sure whether the improved performance justifies the added complexity with recursiveBinaryTree. It could be other techniques yield similar improvements, like memoization or other caching techniques (you sort of do this with the precalculated values for (n < 8). But in these cases we can also keep the memoization separate from the function, you can wrap the function in a memoization function. What do you think?

@bartekleon
Copy link
Contributor Author

bartekleon commented Dec 21, 2019

I am not sure that do you mean by that at all :/
(And the improvement is only visible for big numbers [n>1000/2000])

I have in mind another algorithm which uses this fast factorial and fast doubleFactorial.

And uses property of factorial that you can divide factorial into 2^(n/2), (n/2)! And n!!

For now: current algorithm for factorial is probably the fastest for small numbers and that recursive binary the fastest for big numbers. Although I have not tested all available factorial algorithms on my own (too many of them, and some have super complex algorithms)

@josdejong
Copy link
Owner

I am not sure that do you mean by that at all :/

What I try to say is that I think that I think this second improvement with initializeFactorial and SynchronizedBinaryTree does not outweigh the added complexity.

There is always a trade-off between performance on the one side, and code simplicity and size on the other side. In our case size is definitely relevant when bundling code for browsers. With a simple algorithm you get "good" performance. With a bit smarter algorithm you can get much better performance: that's what your first PR. At some point, to improve the performance even more, you have to write highly complicated algorithms. That's the point where we are with this PR I think. That's the moment you have to take a step back and be critical on whether it really adds value for typical real world scenarios. I think calculating factorial for huge (big)numbers is an edge case that is normally not needed, so I don't think this added complexity is worth it in this case.

For me, a next step would simply be to wrap factorial in a memoization function to improve the performance further (this is up to the user of the library though). See the wikipedia page on memoization which even uses factorial as an example ;).

@bartekleon
Copy link
Contributor Author

bartekleon commented Dec 23, 2019

I might think of adding a memoization table. But I still need a small "starting table" (if you only consider using betterFactorial - [1, 1] is enough (since it doesn't work for n = 0 and n = 1), for initializeFactorial n has to be greater than 7 (or so).

I could also test my another algorithm which uses division of factorial (you can see performance graph for bigints in #1687 [factorial3])

Also notice, that initializeFactorial might be faster for n>1450, but ONLY in BigInts. For BigNumber library it's faster for n>30. Yes, it adds complexity but scales much better than my version of factorial (i still have to check my last implementation though)

@josdejong josdejong merged commit 1f10538 into josdejong:develop Dec 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants