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.

Sign up for GitHub

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

Jump to bottom

Using <Iter>::new instead of exposing internal fields #76787

Merged
merged 3 commits into from
Sep 19, 2020
Merged

Using <Iter>::new instead of exposing internal fields #76787

merged 3 commits into from
Sep 19, 2020

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Sep 16, 2020 •
edited
Loading

As requested in #76311 (comment)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 16, 2020
lcnr
lcnr approved these changes Sep 16, 2020
View reviewed changes
Copy link
Contributor

@lcnr lcnr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, LGTM. It might be better to create the NonNull outside of the new functions so that these functions don't have any additional safety requirements.

library/core/src/slice/iter.rs Outdated Show resolved Hide resolved
lcnr
lcnr reviewed Sep 17, 2020
View reviewed changes
library/core/src/slice/mod.rs Outdated Show resolved Hide resolved
@lcnr
Copy link
Contributor

lcnr commented Sep 17, 2020 •
edited
Loading

slice.iter is quite hot, let's see if we somehow regress something here.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Sep 17, 2020

⌛ Trying commit 75ea5bcb25469d100c5063fa3cf08ede866a2d32 with merge 949e85c6bc2e139a5e9b07b26df01c81c9ecd99e...

@bors
Copy link
Contributor

bors commented Sep 17, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 949e85c6bc2e139a5e9b07b26df01c81c9ecd99e (949e85c6bc2e139a5e9b07b26df01c81c9ecd99e)

@rust-timer
Copy link
Collaborator

Queued 949e85c6bc2e139a5e9b07b26df01c81c9ecd99e with parent 95386b6, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (949e85c6bc2e139a5e9b07b26df01c81c9ecd99e): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@lcnr
Copy link
Contributor

lcnr commented Sep 17, 2020

You might have to put #[inline] on at least some of these functions.

@lcnr
Copy link
Contributor

lcnr commented Sep 17, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Sep 17, 2020

⌛ Trying commit eafe1b365e6784208d7df8462d22da5c2b207f37 with merge d2df5ef8b0f37065ba646a8a28bb6085e0d3b724...

@bors
Copy link
Contributor

bors commented Sep 17, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: d2df5ef8b0f37065ba646a8a28bb6085e0d3b724 (d2df5ef8b0f37065ba646a8a28bb6085e0d3b724)

@rust-timer
Copy link
Collaborator

Queued d2df5ef8b0f37065ba646a8a28bb6085e0d3b724 with parent 95386b6, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (d2df5ef8b0f37065ba646a8a28bb6085e0d3b724): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

lcnr
lcnr reviewed Sep 17, 2020
View reviewed changes
library/core/src/slice/mod.rs Outdated Show resolved Hide resolved
lcnr
lcnr approved these changes Sep 17, 2020
View reviewed changes
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

LGTM

perf also seems fine now 👍

@lcnr
Copy link
Contributor

lcnr commented Sep 18, 2020

Thanks 👍

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Sep 18, 2020

📌 Commit 39732ab8f6e6493079d5e9364521e44d133049fa has been approved by lcnr

@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 Sep 18, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Sep 18, 2020

Did you mark all the file as viewed ? Because I intend to squash some commits together.

@lcnr
Copy link
Contributor

lcnr commented Sep 18, 2020

now I did 😆 go ahead

@tesuji
Copy link
Contributor Author

tesuji commented Sep 18, 2020

Done.

@lcnr
Copy link
Contributor

lcnr commented Sep 18, 2020

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Sep 18, 2020

📌 Commit b659370 has been approved by lcnr

@bors
Copy link
Contributor

bors commented Sep 18, 2020

⌛ Testing commit b659370 with merge c6ab8e5...

@bors
Copy link
Contributor

bors commented Sep 19, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: lcnr
Pushing c6ab8e5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 19, 2020
@bors bors merged commit c6ab8e5 into rust-lang:master Sep 19, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 19, 2020
@tesuji tesuji deleted the slice_iters_new branch September 19, 2020 02:16
@bors bors mentioned this pull request Sep 19, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr approved these changes

Assignees

@lcnr lcnr

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
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.
6 participants
@tesuji @lcnr @rust-timer @bors @rust-highfive @rustbot