-
Notifications
You must be signed in to change notification settings - Fork 147
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
BigInt #120
base: main
Are you sure you want to change the base?
BigInt #120
Conversation
@benrimmington Hmm, your review comments just vanished. I've addressed the copyright header situation. |
Awesome, @xwu. I'll give this a thorough review sometime this week. |
It seems that use of a custom pidigits(10000) BigInt without custom This seems to be competitive with most implementations of BigInt not based on GMP! And we have room to optimize (for instance, by enabling faster multiplication and division). attaswift/BigInt: Earlier observations: pidigits(500) BigInt with custom BigInt with custom attaswift/BigInt: It seems that use of the There's also another order of magnitude of difference in performance that we need to look into. For a fairer comparison, I deleted the division step and disabled Karatsuba multiplication for both implementations. Then I tried swapping out the custom pidigits_without_division(5000) BigInt with custom BigInt without custom attaswift/BigInt (without Karatsuba multiplication): So it seems the idea of keeping the low word separate from the rest of the array is inhibiting performance. The signum-exponent-significand representation, though, seems to be just fine from a performance perspective. |
…d "pidigits" performance test. [BigInt] Disable custom _Significand and Karatsuba multiplication. [BigIntModule] Fix up tests and add "pidigits" performance test.
@xwu Excellent performance! Great job! Any plan to include |
@Sajjon Since a |
@stephentyrone Hmm sorry I might misunderstand you, but if I understand it correctly @xwu excellent work so far just declares attaswift/BigInt contains |
@Sajjon Sure, you can define such a type, but Swift really discourages using unsigned types just for "a value that can never be negative" (see: sizes being |
@stephentyrone Yes I'm quite surprised that Instead of using |
Once you decide to eliminate implicit conversions, this decision follows naturally, because otherwise any form of indexing arithmetic becomes super-cumbersome:
If we had unsigned indices, the closure would either look like this:
or maybe this if you're clever:
These aren't a lot more code, but how they work is significantly less obvious to a new reader. |
@stephentyrone Excellent answer, description and motivation, thank you! @xwu Would be great if the API of |
All implementations are subject to change, I think. There are only seven APIs here that are introduced beyond what's required by existing protocols, and any differences in their naming as compared to attaswift/BigInt are deliberate, often reflecting what's been formalized in the Swift Evolution process since the time that attaswift/BigInt was created as well as the design of other modules in Swift Numerics :) They are subject to further discussion, of course, but I think at this stage what's much more important is nailing down the core implementation. In fact, I hesitated to add any APIs beyond the integer protocols at all for this very reason, because I'd rather not have that discussion quite yet. If the naming of any of the seven APIs I've added is controversial I'd rather strip it from this PR. |
Yeah, the space of integer API is extremely constrained by what's in the stdlib protocols, so there's very little room for variation. |
Ok got it! I saw you've implemented Can't wait to replace (awesome) attaswift/BigInt with just Swift Numerics package everywhere. |
Also, should maybe the BigInt prototype in Swift repo under Prototypes be deleted after this PR is merged? Seems like no need to keep it around? Thoughts on that? :) |
It's used for testing a bunch of Stdlib functionality, so it should stay put for now. |
That and the edge cases - for instance, if the value of the |
I've been working with the code in this PR. I've found a problem in the random number generation. In my case, It turns out the first number occupies 64 bits but the second needs 67 bits. After debugging, in the implementation of I'm very new to Swift and don't yet know how to run the unit tests, but will work on that next. |
While working on a unit test for LCM, I've found a problem with division. The following test snippet fails
|
@swanysteve Yikes, that was a think-o in the implementation, which I've now corrected. |
Thanks @xwu . My LCM and Pow tests now pass. I'm still getting a hang in my random test.
|
|
Sorry, I don't understand. The bug is in |
As you can tell, I haven't been focusing on this much. You're absolutely right; will fix that think-o tonight. Thanks for exercising this code, and keep these coming! |
…values and adding '_isZero'.
… and restore a few stylistic choices.
What's the hold-up on this PR? |
@1oo7 Glad you're enthusiastic about this PR. As you can see, there is some additional work I'd like to do before this gets merged. Karatsuba multiplication is excruciatingly slow, for instance, due to some design decisions that need to be revised. What's more, the PR is waiting on @stephentyrone's comments. Do feel free to contribute any notes about the implementation, particularly bugs you find. However, in general, it's helpful not to bump PRs, and please avoid using the approval features on GitHub as an upvote. |
Thanks Xiaodi. I recognize your name from Swift.org forums were we have had some conversations. Hope to see this progress and also eventually get BigDecimal as well. Thanks for all your hard work. |
Will StaticBigInt change the status of this PR, as in help it progress? https://github.com/apple/swift-evolution/blob/main/proposals/0368-staticbigint.md |
No doubt! |
Here's another stab at implementing a
BigInt
type.Implementation
I started with a sign-magnitude representation, then made a few gradual refinements (sadly, the Git history got nuked somewhere in there):
Taking inspiration from the Java implementation, I switched to storing the signum instead of the sign; this allows us to eliminate the two possible representations of zero.
I decided to include the count of trailing zero words (i.e., an exponent) in a "combination" field, where
_combination = _signum * (_exponent + 1)
. This not only permits constant-time random access of the two's complement representation of negative values, but allows us not to store those trailing zeros. To determine whether a value is a power of two, for instance, it suffices to check whether there is only one nonzero word that has only one nonzero bit.The resultant representation is somewhat reminiscent of IEEE floating point, with each value decomposed into a signum, exponent, and significand such that notionally
value = _signum * _significand << (UInt.bitWidth * _exponent)
. Overall, it seems to meld the best of both worlds (sign-magnitude and two's complement) for the reasons given above. I don't know of another implementation ofBigInt
that takes this approach, so either it's something new and worthwhile or a very silly diversion.Taking @lorentey's point that it is best to have some accommodation for storing smaller numbers without allocating an array, I take a slightly different approach here: instead of storing two words in a tuple and any longer values in an array, this implementation unconditionally stores the low word of the significand inline. In this way, serially incrementing or decrementing even most large values should not trigger a copy-on-write operation.Both schoolbook and Karatsuba multiplication are implemented; currently,
the cutoff is hardcoded at 16 words, but perhaps this should be tunable (although would such an API need to be thread-safe?)[edit: Karatsuba multiplication is temporarily disabled].Bitwise operators perform as they should (working with the logical two's complement representation—in fact, for simplicity, their implementations make use of efficient constant-time random access to the actual two's complement representation that is provided by
Words
).Only a few new APIs that aren't already present on standard library integer types are introduced:
init(words:)
to create a value from a collection ofUInt
-typed words.BinaryInteger
static methodsgcd(_:_:)
(Stein's algorithm),lcm(_:_:)
,pow(_:_:)
(iterative squaring),sqrt(_:)
.BinaryInteger
instance methodinverse(modulo:)
(extended Euclidean algorithm) and static methodpow(_:_:modulo:)
(right-to-left binary method).To-dos:
@inlinable
attribute usage.BigInt
-specific operations (can be done in a follow-up PR).Testing
I have verified that operations currently implemented produce the expected results. Not all edge cases are covered yet, however.
So that I can test calculations using randomly generated operands, I use
attaswift/BigInt
as a reference implementation and check that the current implementation produces the same result as doesattaswift/BigInt
(with one exception, where on manual checking it appears that the latter implementation has a bug).To-dos:
gcd
,lcm
,pow
,sqrt
.attaswift/BigInt
test dependency.