Skip to content
This repository has been archived by the owner on Nov 6, 2023. It is now read-only.

A word-based integer encoding approach. #1

Closed
wants to merge 5 commits into from
Closed

A word-based integer encoding approach. #1

wants to merge 5 commits into from

Conversation

oscbyspro
Copy link

@oscbyspro oscbyspro commented Oct 11, 2023

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...
func testInt256() {
    var value = NBK.blackHoleIdentity(Int256.max) // T.max or T(UInt256.max) for flexible-width
    
    for _ in 0 ..< 250_000 {        
        // numberick:  NBK.blackHole(String(value, radix: 10))
        // stdlib:     NBK.blackHole(String(NBK.someSwiftBinaryInteger(value), radix: 10))
        // foundation: NBK.blackHole(String(bytes: value.numericStringRepresentation, encoding: .ascii))

        NBK.blackHoleInoutIdentity(&value)
    }
}

I have measured it as-is and with everything marked as @inlinable.

Summary

  • [U]Int[8-64]: makes little to no difference.
  • NBKDoubleWidth: 40x faster when private, 1.3x faster when @inlinable.
  • UIntXL (on feature branch): 5.8x faster when private, 3.1x faster when @inlinable.

Foundation: Current

private @inlinable
Int 0.061 0.045
UInt 0.053 0.046
Int256 5.272 0.097
UInt256 4.257 0.091
UIntXL 0.554 0.230

Foundation: After Pull Request

private @inlinable
Int 0.057 0.040
UInt 0.060 0.045
Int256 0.122 0.070
UInt256 0.120 0.072
UIntXL 0.095 0.074

Points of Reference

Stdlib Numberick
Int 0.021 0.023
UInt 0.022 0.022
Int256 12.514 0.065
UInt256 12.477 0.056
UIntXL 3.992 0.058

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.
@oscbyspro
Copy link
Author

oscbyspro commented Oct 11, 2023

Hm. I think I forgot to change the type when testing [U]Int8, so a small integer fast path might still be useful 🧐

@wadetregaskis
Copy link
Owner

wadetregaskis commented Oct 13, 2023

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?

@parkera
Copy link

parkera commented Oct 13, 2023

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.

@glessard
Copy link

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.

@wadetregaskis
Copy link
Owner

wadetregaskis commented Oct 13, 2023

👍

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.

@oscbyspro
Copy link
Author

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?

@wadetregaskis
Copy link
Owner

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.

@oscbyspro
Copy link
Author

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 withUTF8(_:) method. It is not my call to make, but it is a possible design direction.

@wadetregaskis
Copy link
Owner

My first version has now been integrated into upstream (apple/swift-foundation), so you can now send this PR to them.

@oscbyspro
Copy link
Author

Cool. GitHub does not let me change the upstream fork, however. So I suppose I'll close this one, and make a new one.

@oscbyspro oscbyspro closed this Nov 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants