BigInt Fixes for Issues in Violet Tests #262
Conversation
…de faster performance. Removed unnecessary _digits and _digitRadix. Fixed a cut/paste problem in "^=" function. Added test cases for string conversion performance and logical operation tests.
…d into a negative. Fixed division/remainder signs after a multi-word division.
…han the default).
…ersions by 35X to 310X times over original code. New implementation for CustomStringConvertible toString(); speeds conversions by 20X to 90X times over original code.
Sources/BigIntModule/BigInt.swift
Outdated
| @@ -9,6 +9,8 @@ | |||
| // | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| import Foundation | |||
There was a problem hiding this comment.
Numerics can't have a dependency on Foundation for layering reasons. Is it actually used anywhere in this change, or did it just sneak in on accident?
There was a problem hiding this comment.
I needed the log() function to do a quick digit count.
I'll see if this can be done another way.
There was a problem hiding this comment.
Violet does it differently (without log). It uses magic numbers from V8 (google javascrtipt thinge). It is a really good approximation for the upper bound.
Unfortunately it is an upper bound, so sometimes it has a tiny overallocation. This is why after the filling of the buffer there is a if index != -1 check (with comment 'Check for invalid capacity estimation'). If we overallocate then we have to move the whole result (because we are filling from the back). Overallocation is not a problem (because it is tiny), the shift may be because of performance.
Btw. just so we are clear: I confirm that for radix 10 (most common case) this overallocation happens, which means that sometime we have to shift. But even with this penalty the performance is much better than the current main branch.
Btw2. There is a way to avoid this shifting. In Violet if your platform does not have String(unsafeUninitializedCapacity: capacity, initializingUTF8With: initializer) then we implement our own shim. At the end of this shim we call String(cString: ptr) // Borrows 'ptr' to create owned copy.. To avoid shifting we could just increment the ptr so that the buffer starts exactly where the digits start. Though, obviously if your platform has String(unsafeUninitializedCapacity: capacity, initializingUTF8With: initializer) then you have to correctly fill the provided buffer. Anyway it does not really matter...
There was a problem hiding this comment.
Thanks @LiarPrincess for all your comments. Hope you get better soon.
@stephentyrone, I've removed the Foundation dependency by using a table.
|
Just for reference, original Violet code: This is Violet versions have more comments than this PR, so it may be easier to read. I don't want to start a formal review, because I have not compiled your code, but:
It would be nice to run all of the Violet tests from all of the PRs on this code to check if there is something else hiding. I am still under covid, and with my mental-potato-state I would do more harm than good. Even writing this comment took me >1h. I may be able to do this during the weekend. |
Yes, mea culpa, I thought I reviewed the different binary cases and all divided evenly into 64.
No, I was focused on the performance. I'm sure Apple has a tool to do the availability checks for us.
The most common radix in use is 10, so, while there are performance improvements in binary radices, it didn't seem worth all the code.
I agree, I would love to spent all my days making this faster, but this seems quite good compared to what it was. |
|
Hmm... I can't compile the code from this PR: More or less here: if index != -1 {
count = stringMaxSize - index - 1
let dstPtr = buffer.baseAddress!
let srcPtr = dstPtr.advanced(by: index+1)
dstPtr.update(from: srcPtr, count: count) // <-----
dstPtr[count] = 0
}In Violet this code looks like this: // Check for invalid capacity estimation
if index != -1 {
count = capacity - index - 1
let dstPtr = ptr.baseAddress!
let srcPtr = dstPtr.advanced(by: index + 1)
dstPtr.assign(from: srcPtr, count: count) // <-----
// For some reason we have to do this on macOS,
// otherwise the tests will fail (Swift 5.5.2).
dstPtr[count] = 0
}So, the @inlinable
@_alwaysEmitIntoClient
@available(*, deprecated, renamed: "update(from:count:)")
@_silgen_name("_swift_se0370_UnsafeMutablePointer_assign_from_count")
public func assign(from source: UnsafePointer<Pointee>, count: Int) {
update(from: source, count: count)
}Platforms:
In both cases I am using Btw. If you want to have git clone https://github.com/LiarPrincess/swift-numerics.git
cd swift-numerics
git fetch origin biginteger:biginteger
git fetch origin 0-Node:0-Node
git fetch origin 1-Init-from-int:1-Init-from-int
git fetch origin 2-String:2-String
git fetch origin 3-Comparable-equatable-hashable:3-Comparable-equatable-hashable
git fetch origin 4-Unary-PLUS-MINUS-INVERT:4-Unary-PLUS-MINUS-INVERT
git fetch origin 5-Binary-ADD-SUB:5-Binary-ADD-SUB
git fetch origin 6-Binary-MUL-DIV-MOD:6-Binary-MUL-DIV-MOD
git fetch origin 7-Binary-AND-OR-XOR:7-Binary-AND-OR-XOR
git fetch origin 8-Binary-SHIFTS:8-Binary-SHIFTS
git fetch origin 9-Int-properties:9-Int-properties
git fetch origin 10-Copy-on-write:10-Copy-on-write
git fetch origin 11-Property-based-tests:11-Property-based-tests
git fetch origin 12-Apple:12-Apple
git fetch origin 13-Init-from-float:13-Init-from-float
git fetch origin 13-Performance:13-Performance
git fetch origin 15-Codable:15-Codable
git checkout biginteger
git merge --commit --no-edit 0-Node
git merge --commit --no-edit 1-Init-from-int
git merge --commit --no-edit 2-String
git merge --commit --no-edit 3-Comparable-equatable-hashable
git merge --commit --no-edit 4-Unary-PLUS-MINUS-INVERT
git merge --commit --no-edit 5-Binary-ADD-SUB
git merge --commit --no-edit 6-Binary-MUL-DIV-MOD
git merge --commit --no-edit 7-Binary-AND-OR-XOR
git merge --commit --no-edit 8-Binary-SHIFTS
git merge --commit --no-edit 9-Int-properties
git merge --commit --no-edit 10-Copy-on-write
git merge --commit --no-edit 11-Property-based-tests
git merge --commit --no-edit 12-Apple
git merge --commit --no-edit 13-Init-from-float
git merge --commit --no-edit 13-Performance
git merge --commit --no-edit 15-Codable |
|
Oki, when I change Then if we run the full Violet test suite we will get a bunch of failures in The only other failure is in // swift test --enable-test-discovery --filter=BigIntTests.InitFromBinaryFloatingPoint754Tests/test_float80_spiderman
// Skipping the: #if (arch(i386) || arch(x86_64)) && !os(Windows) && !os(Android)
func test_float80_spiderman() {
let d: Float80 = 9223372036854775807.5
let expected: BigInt = BigInt("9223372036854775807")!
let result = BigInt(d)
if result != expected {
print("d: \(d)")
print("expected: \(expected)")
print("result: \(result)")
}
}This will print: Btw. print(Float80.significandBitCount) // 63 - well *teknikly* there is a hidden bit, but whatever...
let int = UInt64(1) << Float80.significandBitCount
let d = Float80(int)
print("int: ", int) // 9223372036854775808
print("Float80: ", d) // 9223372036854775808.0
print("Float80.nextDown:", d.nextDown) // 9223372036854775807.5 - goes down by 0.5 - our test case
print("Float80.nextUp: ", d.nextUp) // 9223372036854775809.0 - goes up by 1I do not think this matters... it is just a fancy floating point trick. This does not happen for Though, I don't think it has to be fixed in this PR. @mgriebling already did a lot of work. It can wait until we merge Violet tests (*if ever*), then we will deal with this failure. |
Added an additional check to isValidPowerOfTwoRadix so odd numbers of bits (except radix 2) don't use the fast path in toString. Now use a table lookup to determine the radix number bits. Added a check for iOS for StringLiteralType. Tested code under iOS and macOS under simulator and x86
A digression: Float80 doesn't have a hidden significand bit; the leading bit is stored explicitly, unlike all other common IEEE 754 formats (https://en.wikipedia.org/wiki/Extended_precision).
And we follow this pattern for Float80 even though the extra bit is stored in that format:
|
|
Yep, I know. This is the reason why we can just be generic over 'floating point' and not 'floating point with a special case for In the comment I wanted to point that 'there is a hidden bit' unlike in other types, though I may have worded that incorrectly/imprecisely. Maybe Also, there are usual checks for the existence of Anyway, the reason why this code snippet does not do what I would expect is: Then Btw. if we change @inlinable
public static func == (lhs: BigInt, rhs: BigInt) -> Bool {
print("lhs: \(lhs.words)")
print("rhs: \(rhs.words)")
return lhs.words == rhs.words
}We will get (just look at the last line): I have not tested this extensively, but any Ubuntu platform that I mentioned above. |
|
Ugh... I forgot to mention that the compilation failure (rename from I can't find in the proposal the exact version of Swift/stdlib where change was implemented (so that we can @inlinable
@_alwaysEmitIntoClient
@available(*, deprecated, renamed: "update(from:count:)")
@_silgen_name("_swift_se0370_UnsafeMutablePointer_assign_from_count")
public func assign(from source: UnsafePointer<Pointee>, count: Int) {
update(from: source, count: count)
}I can confirm that:
Swift 5.8 Release notes mention SE-0370 in 'Swift Evolution Appendix'. Though, I can't 100% confirm that |
|
The whole Vapor/Fibonacci/BigInt thingie on the forum reminded me of this PR. Just so that we are clear: some parts of this PR are uncomfortably similar to Violet code, and I’m not sure if I’m fine with this. |
This update fixes all known issues published by @LiarPrincess (see various pull requests for details)
Improves the LosslessStringConvertible init() run time by 35X (radix 10) and 310X (radix 16).
Improves the CustomStringConvertible toString() run time by 20X (radix 10) and 90X (radix 16).
Test suite has been updated to include tests for all known issues.
Latest update removes Foundation import and fixes additional problems noted by @LiarPrincess.