Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get rid of bounds check in slice::chunks_exact() and related function… #75936

Merged
merged 3 commits into from
Aug 31, 2020

Conversation

sdroege
Copy link
Contributor

@sdroege sdroege commented Aug 26, 2020

…s during construction

LLVM can't figure out in

let rem = self.len() % chunk_size;
let len = self.len() - rem;
let (fst, snd) = self.split_at(len);

and

let rem = self.len() % chunk_size;
let (fst, snd) = self.split_at(rem);

that the index passed to split_at() is smaller than the slice length and
adds a bounds check plus panic for it.

Apart from removing the overhead of the bounds check this also allows
LLVM to optimize code around the ChunksExact iterator better.

@sdroege
Copy link
Contributor Author

sdroege commented Aug 26, 2020

See https://rust.godbolt.org/z/8P74zY for the corresponding assembly of both variants. This is based on the code from #75935, where I noticed this. The variant without bound checks is unrolled by the compiler.

Instead of implementing split_at/split_at_mut manually, it works equally well to simply put a core::intrinsics::assume(len < s.len()) into the code. As I didn't see that used much I didn't do it for this PR but if you prefer that I can switch it over.

Of course even better would be if LLVM would just understand this itself but that seems considerably harder to achieve.

@sdroege sdroege force-pushed the chunks-exact-construction-bounds-check branch from b790acf to a8cd1de Compare August 26, 2020 08:27
@sdroege
Copy link
Contributor Author

sdroege commented Aug 26, 2020

As @lzutao wrote in the other issue, this might also be #74938

@sdroege
Copy link
Contributor Author

sdroege commented Aug 26, 2020

Seems like #74938 makes this one here obsolete. Might still be worth merging depending on when an update to LLVM 12 is planned.

@pickfire
Copy link
Contributor

Weird, how come there is no reviewers assigned automatically? @Dylan-DPC

@Dylan-DPC-zz
Copy link

r? @nagisa

Copy link
Member

@nagisa nagisa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only change I would suggest here is to split these inlined algorithms into a private function (split_at_unchecked/split_at_mut_unchecked), while also possibly taking advantage of them to implement the split_at and split_at_mut as well.

Your call really if you want to do this, r=me otherwise.

@pickfire
Copy link
Contributor

pickfire commented Aug 28, 2020

@nagisa Would it be good if we have split_at_unchecked and split_at_mut_unchecked to make use of get_unchecked on slice?

@nagisa
Copy link
Member

nagisa commented Aug 28, 2020

Yeah, to implement the split_at_{,mut}_unckecked you would use get_unchecked much like its done right now in this PR. split_at{,_mut} functions, if rewritten to use the unchecked variants would then also be changed to contain bound-checking code.

Like I said this is not strictly necessary to land this IMO, and even if we don’t make an adjustment here, we can also do the thing in a follow-up.

@sdroege
Copy link
Contributor Author

sdroege commented Aug 28, 2020

I'd be happy to add the split_at_unchecked() here, easy enough to do so. You'd prefer that over using core::intrinsics::assume() here then.

@sdroege
Copy link
Contributor Author

sdroege commented Aug 28, 2020

split_at_unchecked() is added here now and made use of.

@sdroege sdroege force-pushed the chunks-exact-construction-bounds-check branch 2 times, most recently from cf86779 to 840819b Compare August 28, 2020 07:00
@sdroege sdroege force-pushed the chunks-exact-construction-bounds-check branch from 840819b to 9ad5a85 Compare August 28, 2020 08:42
/// let (left, right) = v.split_at_unchecked(6);
/// assert!(left == [1, 2, 3, 4, 5, 6]);
/// assert!(right == []);
/// }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should show an example of invalid use.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Aug 31, 2020
…ounds-check, r=nagisa

Get rid of bounds check in slice::chunks_exact() and related function…

…s during construction

LLVM can't figure out in

    let rem = self.len() % chunk_size;
    let len = self.len() - rem;
    let (fst, snd) = self.split_at(len);

and

    let rem = self.len() % chunk_size;
    let (fst, snd) = self.split_at(rem);

that the index passed to split_at() is smaller than the slice length and
adds a bounds check plus panic for it.

Apart from removing the overhead of the bounds check this also allows
LLVM to optimize code around the ChunksExact iterator better.
@bors
Copy link
Contributor

bors commented Aug 31, 2020

⌛ Testing commit 8d3cf92 with merge 52d6d94eb07980426e50416d3cf938c5c8335fbb...

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
Build completed successfully in 0:02:12
+ /scripts/validate-toolstate.sh
Cloning into 'rust-toolstate'...
<Nothing changed>
HTTPError: HTTP Error 403: Forbidden
b'{"message":"API rate limit exceeded for user ID 7378925.","documentation_url":"https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting"}'
Traceback (most recent call last):
  File "../../src/tools/publish_toolstate.py", line 288, in <module>
    validate_maintainers(repo, github_token)
  File "../../src/tools/publish_toolstate.py", line 84, in validate_maintainers
    'Accept': 'application/vnd.github.hellcat-preview+json',
  File "/usr/lib/python3.6/urllib/request.py", line 223, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/lib/python3.6/urllib/request.py", line 532, in open
    response = meth(req, response)
  File "/usr/lib/python3.6/urllib/request.py", line 642, in http_response
    'http', request, response, code, msg, hdrs)
  File "/usr/lib/python3.6/urllib/request.py", line 570, in error
    return self._call_chain(*args)
  File "/usr/lib/python3.6/urllib/request.py", line 504, in _call_chain
    result = func(*args)
  File "/usr/lib/python3.6/urllib/request.py", line 650, in http_error_default
    raise HTTPError(req.full_url, code, msg, hdrs, fp)
urllib.error.HTTPError: HTTP Error 403: Forbidden
  local time: Mon Aug 31 05:18:06 UTC 2020
  network time: Mon, 31 Aug 2020 05:18:06 GMT
== end clock drift check ==
== end clock drift check ==
##[error]Process completed with exit code 1.
Terminate orphan process: pid (5093) (node)
Terminate orphan process: pid (5102) (python)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@bors
Copy link
Contributor

bors commented Aug 31, 2020

💔 Test failed - checks-actions

@bors 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 Aug 31, 2020
@sdroege
Copy link
Contributor Author

sdroege commented Aug 31, 2020

The CI failure was some kind of network problem:

b'{"message":"API rate limit exceeded for user ID 7378925.","documentation_url":"https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting"}'

@Dylan-DPC-zz
Copy link

@bors retry

@bors 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 Aug 31, 2020
@bors
Copy link
Contributor

bors commented Aug 31, 2020

⌛ Testing commit 8d3cf92 with merge b4b76c287992b99f73159f22a1482866237c9ebd...

@bors
Copy link
Contributor

bors commented Aug 31, 2020

💔 Test failed - checks-actions

@bors 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 Aug 31, 2020
@sdroege
Copy link
Contributor Author

sdroege commented Aug 31, 2020

Also looks like an unrelated failure

@nagisa
Copy link
Member

nagisa commented Aug 31, 2020

@bors retry

@bors 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 Aug 31, 2020
@bors
Copy link
Contributor

bors commented Aug 31, 2020

⌛ Testing commit 8d3cf92 with merge 897ef3a...

@bors
Copy link
Contributor

bors commented Aug 31, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: nagisa
Pushing 897ef3a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 31, 2020
@bors bors merged commit 897ef3a into rust-lang:master Aug 31, 2020
@sdroege sdroege deleted the chunks-exact-construction-bounds-check branch August 31, 2020 18:14
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 3, 2021
…r=dtolnay

Make `<[T]>::split_at_unchecked` and `<[T]>::split_at_mut_unchecked` public

The methods were originally added in rust-lang#75936 (sdroege@30dc32b), but for some reason as private. Nevertheless, the methods have documentation and even a [tracking issue](rust-lang#76014).

It's very weird to have a tracking issue for private methods and these methods may be useful outside of the standard library. As such, this PR makes the methods public.
@cuviper cuviper added this to the 1.48.0 milestone Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants