[SE-0288] [stdlib] Adding isPower(of:) to BinaryInteger

[dingobye]

This PR presents the implementation of adding a public API isPower(of:) to the BinaryInteger protocol.

• Pitch Thread
• Proposal: SE-0288

[dingobye]

The idea of this implementation is based on a fast-/slow-path pattern. We have a generic implementation, which is suitable for all inputs, as the slow path; and meanwhile provide some particularly optimized implementation for frequently used inputs (e.g., 2 and 10) as the fast paths.

Some experiments (code can be found here) are conducted to compare the performance of different scenarios, including:

• isPower(of:) the public API. It firstly tests if the input base equals 2 or 10. If so, it goes the fast path by calling the corresponding optimized version (i.e., _isPowerOfTwo_*); otherwise, it falls into the slow path (i.e., _slowIsPower(of:)).
• _isPowerOfTwo_words the fast path for input base 2, whose implementation is based on BinaryInteger.words, as suggested by Steve Canon
• _isPowerOfTwo_classic an alternative fast path for input base 2, with the classic implementation n > 0 && n & (n - 1) == 0
• _isPowerOfTwo_ctpop an alternative fast path for input base 2, with the popcount implementation
• _isPowerOfTwo_cttz an alternative fast path for input base 2, with the tzcount implementation
• _isPowerOfTwo_BigInt the fast path for input base 2 optimized for BigInt
• _isPowerOfTen the fast path for input base 10, taken from Michel Fortin's comments
• _isPowerOf(powerOfTwo:) the fast path for input base which itself is a power of 2, taken from Nevin's comments
• _slowIsPower(of:) the generic implementation (i.e., the slow path) for any input

The tables below show the execution time (in sec) for three different types of integers under the -O optimization level across a range of platforms. We focus on Int64 built-in integer, 256-bit DoubleWidth, and 1024-bit BigInt (with 1024-bit width). The results for integers with other bitwidths of the same type have a similar pattern, so they are not discussed here.
Note that the performance of different type implementations varies a lot. So, for different types, the workloads are repeated in different amounts, thus that they can finish within a reasonable duration for measurement purpose. As a result, it does not make sense to directly compare the numbers between different tables.

Table 1. The built-in Int64 type

.	MacBook Pro (2.9G Core i7)	iPhone X Max	iPhone 6S
isPower(of: 2)	0.114	0.120	0.232
_isPowerOfTwo_words	0.113	1.120	0.233
_isPowerOfTwo_classic	0.113	0.120	0.233
_isPowerOfTwo_ctpop	0.154	0.120	0.268
_isPowerOfTwo_cttz	0.161	1.120	0.310
_slowIsPower(of: 2)	1.346	2.352	4.975
isPower(of: 4)	0.083	0.120	0.266
_slowIsPower(of: 4)	1.065	1.364	4.280
_isPowerOfTen	0.243	0.202	0.332
_slowIsPower(of: 10)	0.688	0.997	2.664

Table 2. Prototype DoubleWidth (with Int 256-bit width)

.	MacBook Pro (2.9G Core i7)	iPhone X Max	iPhone 6S
isPower(of: 2)	0.0007	0.0019	0.0036
_isPowerOfTwo_words	0.0007	0.0012	0.0022
_isPowerOfTwo_classic	0.0011	0.0024	0.0052
_isPowerOfTwo_ctpop	0.0011	0.0016	0.0035
_isPowerOfTwo_cttz	0.0010	0.0032	0.0065
_slowIsPower(of: 2)	1.144	0.773	1.930
isPower(of: 4)	0.002	0.003	0.006
_slowIsPower(of: 4)	0.578	0.393	0.972
_isPowerOfTen	0.383	0.272	0.651
_slowIsPower(of: 10)	0.410	0.291	0.681

Table 3. Prototype BigInt (with 1024-bit width)

.	MacBook Pro (2.9G Core i7)	iPhone X Max	iPhone 6S
isPower(of: 2)	0.0014	0.0011	0.0028
_isPowerOfTwo_words	0.0008	0.0006	0.0014
_isPowerOfTwo_classic	0.0034	0.0027	0.0066
_isPowerOfTwo_cttz	0.0019	0.0015	0.0038
_isPowerOfTwo_BigInt	0.0002	0.0002	0.0004
_slowIsPower(of: 2)	1.030	0.779	2.038
isPower(of: 4)	0.002	0.002	0.011
_slowIsPower(of: 4)	0.801	0.598	1.568
_isPowerOfTen	0.608	0.472	1.224
_slowIsPower(of: 10)	0.899	0.677	1.785

It can be seen that:

• isPower(of: 2) (i.e., calling the public interface) has similar performance as the fast path _isPowerOfTwo_words for built-in integers, and reasonably less efficient than the fast path (while still good enough) for the other two prototypes.

• In addition, in terms of performance of those four optimizations for input base 2, _isPowerOfTwo_words is favored, since it is as efficient as isPowerOfTwo_classic for built-in integers, and is more performant than the other three for DoubeWidth and BigInt.

• Moreover, the specifically optimized version for some particular integer type (i.e., _isPowerOfTwo_BigInt) can be significantly more efficient than others.

• Nevertheless, for those optimized versions for some particular inputs (i.e., _isPowerOfTwo, _isPowerOfTen, and _isPowerOf(powerOfTwo:)), they are significantly more performant than the generic implementation (i.e., _slowIsPower(of:)).

The conclusion is the fast-/slow-path pattern is a good fit for implementing this API, given that it happens much more often to check if a number is a power of some popular bases (e.g., 2, 10) than others.

[michelf]

No optimization for bases that are a power of 2?
https://forums.swift.org/t/adding-ispowerof2-to-binaryinteger/24087/31

[dingobye]

Thanks for pointing this out, Michel~ I have put this together and will update the performance results later had the performance results updated. Such optimization is significant in accelerating the isPower(of: 4) case.

[dingobye]

Following @stephentyrone 's advice, I had _isPowerOfTwo improved by using the words-based implementation. It is now more efficient for those prototype integers (i.e. DoubleWidth and BigInt).

[dingobye]

As per the feedbacks from the community, I had the public API isPower(of:) downgraded from a protocol requirement to an extension method, thus that it adds less complexity to the BinaryInteger protocol's interface.

[shahmishal]

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

• How to change the base branch: (Link)
• More detail about the branch update: (Link)

[dingobye]

@shahmishal Thanks for the information, Mishal! I had the base branch changed to main.

@jckarter Hi Joe, I rebased this PR; and added some minor comment, as you suggested in the evolution review, for better clarification purpose. Would you take a look at it and fire a test, please?

[stephentyrone]

@swift-ci test

[stephentyrone]

@swift-ci Please test source compatibility

[stephentyrone]

The comment here doesn't match the implementation. Please update.

[dingobye]

Done!

[dingobye]

Thank Stephen and Ben for your inputs! I had the comments updated.

[benrimmington]

@swift-ci Please test

[benrimmington]

@swift-ci Please test source compatibility

[benrimmington]

All checks have passed (macOS, Linux, Debug, Release).

[dingobye]

Thank you for your help, Ben! I had the PR rebased with commits squashed.

[benrimmington]

@swift-ci Please smoke test

[mdznr]

Suggested change

	// Return true if the product eventualy hits bound. Because if bound is
	// Return true if the product eventually hits bound. Because if bound is

[dingobye]

Nice catch, Matt~

[stephentyrone]

So basically this looks fine to me, except for a weird process detail: during the time period between when this proposal was written and when the SE approval actually ran, Swift Preview was introduced and the process changed, meaning that this feature should actually land first in Swift Preview rather than directly in the standard library.

It's annoying because while Swift Preview is supposed to decrease the workload for proposals, it increases it for things that happen to have been inflight while the process moved under them. My apologies for that. Please reach out to me and @natecook1000 if you need any assistance in creating a PR against preview.

[dingobye]

@stephentyrone Thanks for pulling this work on the right track, Stephen!

I created a PR against the staging repo here, with most of copy&paste work.

As for the preview repo, looks like we need to have such feature setting up its own repository before re-exporting it from the umbrella library. Is there anything I should do to proceed with the preview repo?

[stephentyrone]

@natecook1000 Can you clarify the process for @dingobye?

[stephentyrone]

@dingobye There's one other thing we should resolve (which I should have flagged in the proposal review, but missed). Instead of the existing signature:

public func isPower(of base: Self) -> Bool

this method should be:

public func isPower<Base: BinaryInteger>(of base: Base) -> Bool

This is a fairly trivial change, but will allow us to avoid creating a bignum object in the case where Self is an arbitrary-precision (or even just large, e.g. Int128 or bigger) integer and base is small. Formally, this requires another evolution proposal, but it can be quite perfunctory, and I'm happy to help write the proposal. I think you should go ahead and adopt this change for your preview package, and we'll get review started as soon as we can.

[dingobye]

@stephentyrone Hi Stephen, I updated the PR against the staging repo here as per your advice.

In fact, I took some time thinking about the function signature at the very beginning when pitching this idea. After browsing the existing functions in standard library, I made the decision to use Self for consistency consideration. Because standard library is mostly using Self for interface functions, with some examples below:

func quotientAndRemainder(dividingBy: Self) -> (quotient: Self, remainder: Self)
func isMultiple(of: Self) -> Bool
static func &<<= (inout Self, Self)
static func random(in: ClosedRange<Self>) -> Self

Pros of using base: Self:

• consistent with most other functions
• users (at least myself) may feel more natural to see it Self

Cons of using base: Self:

• non-optimal performance when Self is heavy and base is in some lightweight type. However, such kind of limitation also exists in many other standard library functions (e.g., the above shifting operation when shifting 1 bit on a QuadrupleWidth). Is it tolerable?

I don't mind going through review process, since the public function signature is so important to ABI. Not sure if it is really worth the change. Would like to hear more of your inputs.

[stephentyrone]

However, such kind of limitation also exists in many other standard library functions (e.g., the above shifting operation when shifting 1 bit on a QuadrupleWidth).

Well, no, because we also have

static func &<<= <Other: BinaryInteger>(inout Self, Other) -> Self

isMultiple should receive the same treatment as isPower, and that can be folded into the proposal (we'll have to keep the old version around for binary compatibility reasons, but we can still add the generic one).

random(in:) and quotientAndRemainder(dividingBy:) are primarily used in ways that make it so that having them be generic makes a lot less sense, so those are fine as is.

[dingobye]

@stephentyrone

Well, no, because we also have
static func &<<= <Other: BinaryInteger>(inout Self, Other) -> Self

If we check its implementation, it turns rhs to Self via Self(truncatingIfNeeded: rhs). So It does create a QuadrupleWidth for 1, when shifting 1 bit on a QuadrupleWidth. It is non-optimal for lightweight Other type, which is the same as this PR. Anyway, having such function signature preserves the possibility/potential for future optimizations without much concern about impacts on ABI.

Can we revise SE-0288 for a 2nd round review, without having to prepare a separate proposal? If it works, would you help revise the proposal and staging repo's implementation to make it armed for a 2nd round review? BTW, don't forget to remove the first acknowledgement item when adding your name into the author list. I've already owned you too much on this story, and you are well deserved. Thank you~

[tbkka]

Is this PR still live? Or has it been supplanted by a PR to the Preview repo?

[shgew]

🔨

Suggested change

	/// Returns `true` iff `self` is a power of the given `base`.
	/// Returns `true` if `self` is a power of the given `base`.