Add "integer square root" method to integer primitive types by FedericoStra · Pull Request #116176 · rust-lang/rust

FedericoStra · 2023-09-26T14:29:05Z

For every suffix N among 8, 16, 32, 64, 128 and size, this PR adds the methods

constfn uN::isqrt() -> uN;constfn iN::isqrt() -> iN;constfn iN::checked_isqrt() -> Option<iN>;

to compute the integer square root, addressing issue #89273.

The implementation is based on the base 2 digit-by-digit algorithm on Wikipedia, which after some benchmarking has proved to be faster than both binary search and Heron's/Newton's method. I haven't had the time to understand and port this code based on lookup tables instead, but I'm not sure whether it's worth complicating such a function this much for relatively little benefit.

rustbot · 2023-09-26T14:29:13Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cuviper (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

joboet

Looks great to me, except for some small nits. Also, could you file an API change proposal for this method? That will allow separating discussions about the API from the implementation review.

@rustbot label +T-libs-api +S-waiting-on-ACP

cuviper · 2023-09-26T16:49:25Z

r? libs-api

dtolnay · 2023-09-28T00:31:50Z

This API looks good to me. I'd be all right landing this without ACP. But if you already have one in progress, feel free to post and I'll approve it.

Please open a tracking issue using the Library Tracking Issue template (https://github.com/rust-lang/rust/issues/new/choose) and update the stability attribute as shown in #116176 (comment).

Fix C-ism and type inference.

FedericoStra · 2023-09-28T10:54:03Z

This API looks good to me. I'd be all right landing this without ACP. But if you already have one in progress, feel free to post and I'll approve it.

I'm not familiar with the ACP procedure (first time contributing) and I don't have one ready yet; however if you change idea and want me to create one I'll try to produce something decent.

Please open a tracking issue using the Library Tracking Issue template (https://github.com/rust-lang/rust/issues/new/choose) and update the stability attribute as shown in #116176 (comment).

I've opened the tracking issue #116226 and marked the methods as

#[unstable(feature = "isqrt", issue = "116226")]#[rustc_const_unstable(feature = "isqrt", issue = "116226")]

rust-lang#89273 (comment)

dtolnay

Thanks!

dtolnay · 2023-09-28T16:14:23Z

@bors r+

bors · 2023-09-28T16:14:25Z

📌 Commit 25648de has been approved by dtolnay

It is now in the queue for this repository.

bors · 2023-09-28T19:40:43Z

⌛ Testing commit 25648de with merge 7defbf2...

Add "integer square root" method to integer primitive types For every suffix `N` among `8`, `16`, `32`, `64`, `128` and `size`, this PR adds the methods ```rust const fn uN::isqrt() -> uN; const fn iN::isqrt() -> iN; const fn iN::checked_isqrt() -> Option<iN>; ``` to compute the [integer square root](https://en.wikipedia.org/wiki/Integer_square_root), addressing issue rust-lang#89273. The implementation is based on the [base 2 digit-by-digit algorithm](https://en.wikipedia.org/wiki/Methods_of_computing_square_roots#Binary_numeral_system_(base_2)) on Wikipedia, which after some benchmarking has proved to be faster than both binary search and Heron's/Newton's method. I haven't had the time to understand and port [this code](http://atoms.alife.co.uk/sqrt/SquareRoot.java) based on lookup tables instead, but I'm not sure whether it's worth complicating such a function this much for relatively little benefit.

bors · 2023-09-28T19:43:10Z

💔 Test failed - checks-actions

rust-log-analyzer · 2023-09-28T20:11:51Z

A job failed! Check out the build log: (web)(plain)

Click to see the possible cause of the failure (guessed by this bot)


Sending build context to Docker daemon 432.1kB
Step 1/14 : FROM ubuntu:22.04
Head "https://registry-1.docker.io/v2/library/ubuntu/manifests/22.04": EOF
DEPRECATED: The legacy builder is deprecated and will be removed in a future release.
BuildKit is currently disabled; enable it by removing the DOCKER_BUILDKIT=0
environment-variable.
Sending build context to Docker daemon 432.1kB
Step 1/14 : FROM ubuntu:22.04
Head "https://registry-1.docker.io/v2/library/ubuntu/manifests/22.04": Get "https://auth.docker.io/token?account=githubactions&scope=repository%3Alibrary%2Fubuntu%3Apull&service=registry.docker.io": EOF
DEPRECATED: The legacy builder is deprecated and will be removed in a future release.
BuildKit is currently disabled; enable it by removing the DOCKER_BUILDKIT=0
environment-variable.
Sending build context to Docker daemon 432.1kB
Step 1/14 : FROM ubuntu:22.04
Head "https://registry-1.docker.io/v2/library/ubuntu/manifests/22.04": Get "https://auth.docker.io/token?account=githubactions&scope=repository%3Alibrary%2Fubuntu%3Apull&service=registry.docker.io": EOF
DEPRECATED: The legacy builder is deprecated and will be removed in a future release.
BuildKit is currently disabled; enable it by removing the DOCKER_BUILDKIT=0
environment-variable.
Sending build context to Docker daemon 432.1kB
Step 1/14 : FROM ubuntu:22.04
Get "https://registry-1.docker.io/v2/": EOF
DEPRECATED: The legacy builder is deprecated and will be removed in a future release.
BuildKit is currently disabled; enable it by removing the DOCKER_BUILDKIT=0
environment-variable.
Sending build context to Docker daemon 432.1kB
Step 1/14 : FROM ubuntu:22.04
Get "https://registry-1.docker.io/v2/": EOF
##[error]Process completed with exit code 1.
Post job cleanup.

dtolnay · 2023-09-28T20:14:17Z

 Get "https://registry-1.docker.io/v2/": EOF The command has failed after 5 attempts.

Looks like a flake.

@bors retry

bors · 2023-09-29T07:35:48Z

⌛ Testing commit 25648de with merge b8536c1...

bors · 2023-09-29T09:23:25Z

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing b8536c1 to master...

rust-timer · 2023-09-29T10:44:33Z

Finished benchmarking commit (b8536c1): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.6%	[0.4%, 0.7%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 632.014s -> 631.581s (-0.07%)
Artifact size: 317.22 MiB -> 317.22 MiB (0.00%)

FedericoStra added 4 commits September 22, 2023 16:11

isqrt: initial implementation
3e8676c

isqrt: add tests
1b34f1c

isqrt: remove duplication by delegating to unsigned integers
68f0b47

isqrt: add more tests
d49da0f

rustbot assigned cuviper Sep 26, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 26, 2023

joboet requested changes Sep 26, 2023
View reviewed changes

rustbot added S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. T-libs-api [DEPRECATED; DO NOT USE] labels Sep 26, 2023

rustbot assigned dtolnay and unassigned cuviper Sep 26, 2023

dtolnay reviewed Sep 28, 2023
View reviewed changes

Comment threadlibrary/core/src/num/uint_macros.rs

FedericoStra added 3 commits September 28, 2023 11:39

fixup! isqrt: initial implementation
17dfb18
Fix C-ism and type inference.

isqrt: fix stability
c97ab23

isqrt: cite source and rename variables to match original C code
5146317

FedericoStra mentioned this pull request Sep 28, 2023
Tracking Issue for {u8,i8,...}::isqrt#116226
Closed
3 tasks

fixup! isqrt: fix stability
77f9eae

isqrt: assume that isqrt takes half as many bits
fcdfd5b
rust-lang#89273 (comment)

dtolnay requested changes Sep 28, 2023
View reviewed changes

Comment threadlibrary/core/tests/num/int_macros.rs

isqrt: disable long running tests in Miri
25648de

FedericoStra requested a review from dtolnay September 28, 2023 16:01

dtolnay approved these changes Sep 28, 2023
View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 28, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 28, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 28, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 29, 2023

bors merged commit b8536c1 into rust-lang:masterSep 29, 2023

rustbot added this to the 1.74.0 milestone Sep 29, 2023

bors mentioned this pull request Sep 29, 2023
Implement strict integer operations that panic on overflow #116090
Merged

pvdrz mentioned this pull request Oct 4, 2023
Fix divergence from upstream masterferrocene/ferrocene#23
Merged

ChaiTRex mentioned this pull request Aug 31, 2024
FCP WIP ChaiTRex/rust#1
Closed

Uh oh!

Uh oh!

Conversation

FedericoStra commented Sep 26, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Sep 26, 2023

Uh oh!

joboet left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cuviper commented Sep 26, 2023

Uh oh!

Uh oh!

dtolnay commented Sep 28, 2023

Uh oh!

FedericoStra commented Sep 28, 2023

Uh oh!

Uh oh!

dtolnay left a comment

Choose a reason for hiding this comment

Uh oh!

dtolnay commented Sep 28, 2023

Uh oh!

bors commented Sep 28, 2023

Uh oh!

bors commented Sep 28, 2023

Uh oh!

bors commented Sep 28, 2023

Uh oh!

rust-log-analyzer commented Sep 28, 2023

Uh oh!

dtolnay commented Sep 28, 2023

Uh oh!

bors commented Sep 29, 2023

Uh oh!

bors commented Sep 29, 2023

Uh oh!

rust-timer commented Sep 29, 2023

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

FedericoStra commented Sep 26, 2023•
edited
Loading

joboet left a comment •
edited
Loading