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

BigInt #120

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

BigInt #120

wants to merge 16 commits into from

Conversation

xwu
Copy link
Contributor

@xwu xwu commented Jun 8, 2020

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 of BigInt 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:

  • The initializer init(words:) to create a value from a collection of UInt-typed words.
  • The BinaryInteger static methods gcd(_:_:) (Stein's algorithm), lcm(_:_:), pow(_:_:) (iterative squaring), sqrt(_:).
  • The BinaryInteger instance method inverse(modulo:) (extended Euclidean algorithm) and static method pow(_:_:modulo:) (right-to-left binary method).

To-dos:

  • Implement division (Knuth Algorithm D; Burnikel-Ziegler can be done in a follow-up PR).
  • Update README.
  • Audit @inlinable attribute usage.
  • Implement the most salient 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 does attaswift/BigInt (with one exception, where on manual checking it appears that the latter implementation has a bug).

To-dos:

  • Test random number generation APIs, gcd, lcm, pow, sqrt.
  • Compare performance.
  • Consider stripping attaswift/BigInt test dependency.

@xwu
Copy link
Contributor Author

xwu commented Jun 9, 2020

@benrimmington Hmm, your review comments just vanished. I've addressed the copyright header situation.

@xwu xwu changed the title [Sketch] BigInt BigInt Jun 9, 2020
@stephentyrone
Copy link
Member

Awesome, @xwu. I'll give this a thorough review sometime this week.

@xwu
Copy link
Contributor Author

xwu commented Jun 10, 2020

It seems that use of a custom BigInt._Significand and Slice<BigInt._Significand> (during Karatsuba multiplication) are major bottlenecks. Once both were disabled, and with compiler optimization and all methods inlinable, we get something like this:

pidigits(10000)

BigInt without custom _Significand (without Karatsuba multiplication):
average: 5.226, relative standard deviation: 0.322%

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:
average: 10.745, relative standard deviation: 0.223%


Earlier observations:
Karatsuba multiplication is in need of a great deal more optimization; benchmarking pidigits with optimizations on and all methods inlinable, we get something like this on my machine:

pidigits(500)

BigInt with custom _Significand (with Karatsuba multiplication):
average: 2.142, relative standard deviation: 1.434%

BigInt with custom _Significand (without Karatsuba multiplication):
average: 0.230, relative standard deviation: 4.056%

attaswift/BigInt:
average: 0.030, relative standard deviation: 21.966%

It seems that use of the Slice type (which I rely on heavily for Karatsuba multiplication) is quite suboptimal.

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 _Significand for just [UInt]:

pidigits_without_division(5000)

BigInt with custom _Significand (without Karatsuba multiplication):
average: 1.749, relative standard deviation: 0.589%

BigInt without custom _Significand (without Karatsuba multiplication):
average: 0.134, relative standard deviation: 7.622%

attaswift/BigInt (without Karatsuba multiplication):
average: 0.239, relative standard deviation: 2.283%

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.
@Sajjon
Copy link
Contributor

Sajjon commented Jun 23, 2020

@xwu Excellent performance! Great job! Any plan to include BigUInt as well? :)

@stephentyrone
Copy link
Member

@Sajjon Since a BigInt, by definition, can never overflow, there's really no such thing as a BigUInt (the distinction between signed and unsigned only makes sense for fixed-width types).

@Sajjon
Copy link
Contributor

Sajjon commented Jun 23, 2020

@stephentyrone Hmm sorry I might misunderstand you, but if I understand it correctly @xwu excellent work so far just declares BigInt which is signed and thus can allow for negative values. But exactly the same use cases using Swift Foundation where I might want to model my data with UInt instead of Int to mark that the value cannot and should not ever be negative, I would like to do the same thing using Big integers, but at compile time, disallow for negative values.

attaswift/BigInt contains BigUInt which I use today :)

@stephentyrone
Copy link
Member

@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 Int instead of UInt). For fixed-width types, there are other differences that justify having the unsigned types (the arithmetic operations and shifts have different semantics, the set of representable values is different, even if you restrict to all-positive values), but none of those differences hold for non-fixed-width types.

@Sajjon
Copy link
Contributor

Sajjon commented Jun 23, 2020

@stephentyrone Yes I'm quite surprised that Int is the default type of Swift Array's size and also Indices... What is the index -1? It makes sense as an ephemeral value, like, subtract 1 from an index, but only during that subtraction, which more elegantly is expressed as a throwing subtraction function between UInt. So I'm super curious, why did Int become the default integer type, instead of UInt, at least in the context of Array/Collections? And is that (the context of Arrays/Collections) the only reason why "Swift really discourages using unsigned types just for...." ?

Instead of using UInt for disallowing negative values one can of course wrap it in a custom struct, but since Swift lacks Haskell's newtype functionality and since we do not have easy way to delegate conformance of protocols, such as FixedWidthInteger (and all other relevant numerical protocols) it is really cumbersome to wrap values if one still wants to use integer functionality on this new type.

@stephentyrone
Copy link
Member

So I'm super curious, why did Int become the default integer type, instead of UInt, at least in the context of Array/Collections? And is that (the context of Arrays/Collections) the only reason why "Swift really discourages using unsigned types just for...." ?

Once you decide to eliminate implicit conversions, this decision follows naturally, because otherwise any form of indexing arithmetic becomes super-cumbersome:

  • Suppose I don't know the relative order of i and j, but want to know how far apart they are. If everything is signed, even though they're "always positive", I do abs(i - j). To work with unsigned indices, I would have to use i < j ? j - i : i - j or UInt(abs(Int(i) - Int(j))).

  • More generally, being able to unconditionally do arithmetic that forms a possibly-out-of-range index, and then check if its in-range before use, is often a much simpler idiom that needing to detect conditions that could lead to forming an out-of-range index before I do it. Consider a really simple convolution with zero extension:

for i in a.indices {
  a[i] = (0 ... 5).reduce(into: 0) {
    let j = i - 2 + $1
    if a.indices.contains(j) { $0 += weights[$1]*a[j] }
  }
}

If we had unsigned indices, the closure would either look like this:

    let j = i + $1
    if j >= 2 || j < a.count + 2 { // can't subtract 2 from `j` until we know it's >= 2.
      $0 += weights[$1]*a[j - 2]
    }

or maybe this if you're clever:

    let j = i + $1 &- 2 // if this wraps, we'll fail the check below
    if j < a.count {
      $0 += weights[$1]*a[j]
    }

These aren't a lot more code, but how they work is significantly less obvious to a new reader.

@Sajjon
Copy link
Contributor

Sajjon commented Jun 25, 2020

@stephentyrone Excellent answer, description and motivation, thank you!

@xwu Would be great if the API of BigInt could somewhat reflect the one of attaswift/BigInt- it would allow for easy transition - I can help out id you want to? :) Just let me know when the current implementation is somewhat stable (or maybe it is?).

@xwu
Copy link
Contributor Author

xwu commented Jun 25, 2020

@xwu Would be great if the API of BigInt could somewhat reflect the one of attaswift/BigInt- it would allow for easy transition - I can help out id you want to? :) Just let me know when the current implementation is somewhat stable (or maybe it is?).

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.

@stephentyrone
Copy link
Member

There are only seven APIs here that are introduced beyond what's required by existing protocols

Yeah, the space of integer API is extremely constrained by what's in the stdlib protocols, so there's very little room for variation.

@Sajjon
Copy link
Contributor

Sajjon commented Jun 25, 2020

Ok got it! I saw you've implemented pow(_ base: Self, _ exponent: Self, modulo modulus: Self) -> Self? which is great 👍, I've used that a lot in attaswift/BigInt when implementing ECC.

Can't wait to replace (awesome) attaswift/BigInt with just Swift Numerics package everywhere.

@Sajjon
Copy link
Contributor

Sajjon commented Jun 25, 2020

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? :)

@stephentyrone
Copy link
Member

It's used for testing a bunch of Stdlib functionality, so it should stay put for now.

@dwaite
Copy link

dwaite commented Jul 27, 2020

  • Suppose I don't know the relative order of i and j, but want to know how far apart they are. If everything is signed, even though they're "always positive", I do abs(i - j). To work with unsigned indices, I would have to use i < j ? j - i : i - j or UInt(abs(Int(i) - Int(j))).

That and the edge cases - for instance, if the value of the UInts i or j is greater than Int.max, the difference will overflow. Since you may be already be using the 64bit unsigned integer type, dealing with that edge case requires more work. Work that you don't typically deal with in C (not that the compiler there will help you by doing the "right" thing).

@swanysteve
Copy link

I've been working with the code in this PR. I've found a problem in the random number generation.

In my case,
BigInt.random(in: BigInt(0)...BigInt("12345678901234567890")! )
works but
BigInt.random(in: BigInt(0)...BigInt("98765432109876543210")! )
hangs.

It turns out the first number occupies 64 bits but the second needs 67 bits.

After debugging, in the implementation of
extension RandomNumberGenerator { @usableFromInline internal mutating func _next(upperBound: BigInt) -> BigInt {
in RandomNumberGenerator.swift,
the two cases where results are OR-ed with mask should instead be AND-ed with mask. I have seen the repeat loop run multiple times, but I no longer see a hang.

I'm very new to Swift and don't yet know how to run the unit tests, but will work on that next.

@swanysteve
Copy link

While working on a unit test for LCM, I've found a problem with division.

The following test snippet fails

    let s = "3182990411991163758463334586237477812181413821081264903177942613807541528844305823312360947978873366530408681494780"
    let t = "16012987498029214696648267754993196043335730812460137832692217701508007910953126091749743662257786118530085930814191"
    let m = (BigInt(s)!, AttaswiftBigInt(s)!)
    let n = (BigInt(t)!, AttaswiftBigInt(t)!)
    XCTAssertEqual((m.0*n.0) / m.0, BigInt((m.1*n.1) / m.1))

@xwu
Copy link
Contributor Author

xwu commented Oct 8, 2020

@swanysteve Yikes, that was a think-o in the implementation, which I've now corrected.

@xwu xwu changed the base branch from master to main October 9, 2020 00:51
@swanysteve
Copy link

Thanks @xwu . My LCM and Pow tests now pass.

I'm still getting a hang in my random test.

  func testRandom() {
    let b1 = BigInt(0)
    let t1 = BigInt("12345678901234567890")! //64 bits
    let t2 = BigInt("98765432109876543210")! //67 bits

    func testClosedRanges() {
      let n1 = BigInt.random(in: b1...t1)
      XCTAssert( n1 >= b1 && n1 <= t1);
      let n2 = BigInt.random(in: b1...t2)
      XCTAssert( n2 >= b1 && n2 <= t2);
    }

    func testOpenRanges() {
      let n1 = BigInt.random(in: b1..<t1)
      XCTAssert( n1 >= b1 && n1 < t1);
      let n2 = BigInt.random(in: b1..<t2)
      XCTAssert( n2 >= b1 && n2 < t2);
    }

    testClosedRanges()
    testOpenRanges()
  }

@xwu
Copy link
Contributor Author

xwu commented Oct 9, 2020

As you've noted, that's a standard library bug :)

@swanysteve
Copy link

Sorry, I don't understand. The bug is in BigIntModule/RandomNumberGenerator.swift which is part of this PR, AFAICT.

@xwu
Copy link
Contributor Author

xwu commented Oct 9, 2020

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!

@1oo7
Copy link

1oo7 commented Feb 6, 2021

What's the hold-up on this PR?

@xwu
Copy link
Contributor Author

xwu commented Feb 6, 2021

@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.

@1oo7
Copy link

1oo7 commented Feb 9, 2021

@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.

@Sajjon
Copy link
Contributor

Sajjon commented Sep 16, 2022

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

@xwu
Copy link
Contributor Author

xwu commented Sep 16, 2022

No doubt!

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.

6 participants