-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
These changes aim to achieve two things. The first is making the algorithm non-generic, so DoubleWidth performs well. The second is performing mutating division, which makes arbitrary precision faster. Copying the binary integer's words to a temporary allocation achieves both. Since the algorithm takes a buffer pointer, it is also possible to test big integers without big integer models.
Hm. I think I forgot to change the type when testing [U]Int8, so a small integer fast path might still be useful 🧐 |
Very cool - thank you for diving into this and making such big optimisations! It looks like the resulting code is actually a bit simpler, too - bonus! Procedurally I'm not sure what the best way to proceed is - whether we should review this here and integrate it into my [temporary] fork before going upstream, or whether to just integrate my first patch and then you could pull request against upstream directly? My patch seems to be basically reviewed & accepted - I'm not actually sure what the hold up is on integrating it - so maybe cleaner to wrap that up and do these optimisations as a second patch? I think the latter probably makes more sense, as this is going to be have to be reviewed by the Foundation owners anyway, and they might have opinions on some of the trade-offs involved. What do you think, @oscbyspro? Reviewers of my initial change - @parkera, @iCharlesHu, @glessard, @itingliu, @stephentyrone, @tbkka - do you have any preferences in this regard? |
I agree with the direction of doing it as a second patch. I think we're close to merging the first one, actually. It's really exciting to see so much progress on this implementation. Thank you both for your work on it. |
The policy of the swift-foundation repo is to do squash commits. This would muddle your contributions if you have a joint PR. If that's not a concern you can go joint, but if it is (and that's fine) we can handle multiple PRs. |
👍 I'm not too worried about attribution (re. my ego), but it's probably wise to keep things clean & accurate in that regard for the sake of people looking back from the future. Unless I've overlooked something - I'm still getting to grips with GitHub's code review GUI, which I find very unintuitive - the first patch is just waiting on @glessard & @iCharlesHu to follow up on some of their earlier comments. Let's try and get that first patch in promptly now - I don't want to hold up @oscbyspro's work. |
Sure. I can open a second pull request in Foundation's repository. I didn't want to side-step @wadetregaskis's pending contribution and wasn't sure how to go about it. Do you prefer that I wait until it is resolved or that I pull request it as-is? |
Probably easiest to just wait until the first patch is integrated, then you can (with any luck) just change your upstream to apple/swift-foundation and not have to rebase. |
I did not touch the proposed API. In light of the "short Array optimization" forum post, however, I could rewrite it so the "numeric string representation" returns a String rather than an ArraySlice. The ASCII buffer would then be accessed using its |
My first version has now been integrated into upstream (apple/swift-foundation), so you can now send this PR to them. |
Cool. GitHub does not let me change the upstream fork, however. So I suppose I'll close this one, and make a new one. |
Hi, I appreciate your work!
I've spent some time on an alternative approach with measurements.
Description
These changes aim to achieve two things. The first is making the algorithm non-generic, so DoubleWidth performs well. The second is performing mutating division, which makes arbitrary precision faster. Copying the binary integer's words to a temporary allocation achieves both. Since the algorithm takes a buffer pointer, it is also possible to test big integers without big integer models.
Measurements
The tests were of the following shape, using T.max or T(UInt256.max).
Tests...
I have measured it as-is and with everything marked as
@inlinable
.Summary
[U]Int[8-64]: makes little to no difference.@inlinable
.@inlinable
.Foundation: Current
@inlinable
Foundation: After Pull Request
@inlinable
Points of Reference