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

Split core::slice to smaller mods #76311

Merged
merged 6 commits into from
Sep 15, 2020
Merged

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Sep 4, 2020

Unfortunately the #[lang = "slice"] is too big (3003 lines), I cannot split it further.

Note for reviewer:

  • I split to multiple commits for easier reviewing, but I could git squash them all to one if requested.
  • Recommend pulling this change locally and using advanced git diff viewer or this command:
    git show --reverse --color-moved=dimmed-zebra master..
    

I split core/slice/mod.rs to these modules:

  • ascii: For operations on [u8].
  • cmp: For comparison operations on [T], like PartialEq and SliceContains impl.
  • index: For indexing operations like Index/IndexMut and SliceIndex.
  • iter: For Iterator definitions and implementation on [T].
    • macros: For iterator! and forward_iterator! macros.
  • raw: For free function to create &[T] or &mut [T] from pointer + length or a reference.

The heapsort wrapper in mod.rs is removed in favor of reexport from sort::heapsort.

@tesuji
Copy link
Contributor Author

tesuji commented Sep 4, 2020

cc #60302
r? @Mark-Simulacrum for review or reassignment
@rustbot modify labels: +S-waiting-on-review

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 4, 2020
library/core/src/slice/raw.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Sep 4, 2020
@bors
Copy link
Contributor

bors commented Sep 4, 2020

☔ The latest upstream changes (presumably #75207) made this pull request unmergeable. Please resolve the merge conflicts.

@pickfire
Copy link
Contributor

pickfire commented Sep 4, 2020

Interesting, I didn't know about --color-moved=dimmed-zebra.

@tesuji tesuji force-pushed the split_core-slice branch 3 times, most recently from 2d15cfe to a485264 Compare September 7, 2020 02:47
@bors
Copy link
Contributor

bors commented Sep 10, 2020

☔ The latest upstream changes (presumably #76540) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@tesuji
Copy link
Contributor Author

tesuji commented Sep 10, 2020

Rebased.

@tesuji
Copy link
Contributor Author

tesuji commented Sep 11, 2020

r? @lcnr

@bors
Copy link
Contributor

bors commented Sep 12, 2020

☔ The latest upstream changes (presumably #75021) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@tesuji
Copy link
Contributor Author

tesuji commented Sep 13, 2020

Rebased.

@bors
Copy link
Contributor

bors commented Sep 14, 2020

☔ The latest upstream changes (presumably #76678) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@tesuji
Copy link
Contributor Author

tesuji commented Sep 14, 2020

Rebased.

@tesuji
Copy link
Contributor Author

tesuji commented Sep 14, 2020

Rebasing merge conflicts in these pull requests is kinda painful without being reviewed.
I have to make sure the rebase don't miss any details.
I am going to close these PRs and tell the associated issue #60302 that it is a won't fix issue.

@lcnr
Copy link
Contributor

lcnr commented Sep 14, 2020

will try to review this today, sorry for being silent this weekend

@bors
Copy link
Contributor

bors commented Sep 15, 2020

📌 Commit 6655ad7 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 15, 2020
@bors
Copy link
Contributor

bors commented Sep 15, 2020

⌛ Testing commit 6655ad7 with merge 4aa7dc2038d4fe5064c83fe5737e3b7c0d086829...

@bors
Copy link
Contributor

bors commented Sep 15, 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 Sep 15, 2020
@lcnr
Copy link
Contributor

lcnr commented Sep 15, 2020

I think you have to update

pub const SLICE_ITER: [&str; 3] = ["core", "slice", "Iter"];
to fix this.

@tesuji
Copy link
Contributor Author

tesuji commented Sep 15, 2020

Oh, my fool. Thought that a spurious failure in the previous error.
Filed rust-lang/rust-clippy#6047 to improve the situation.

@lcnr
Copy link
Contributor

lcnr commented Sep 15, 2020

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Sep 15, 2020

📌 Commit c65050d 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 15, 2020
@bors
Copy link
Contributor

bors commented Sep 15, 2020

⌛ Testing commit c65050d with merge 4c1966f...

@bors
Copy link
Contributor

bors commented Sep 15, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 15, 2020
@bors bors merged commit 4c1966f into rust-lang:master Sep 15, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 15, 2020
@tesuji tesuji deleted the split_core-slice branch September 15, 2020 14:28
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 18, 2020
Using <Iter>::new instead of exposing internal fields

As requested in rust-lang#76311 (comment)
@ecstatic-morse
Copy link
Contributor

Hi! This PR showed up in the weekly perf triage report. It resulted in a moderate improvement in instruction counts (up to -1.9% on full builds of html5ever-opt). However, most other benchmarks regressed very slightly.

I've observed some other module splitting PRs that have unexpected effects on performance. I wonder why that is? In any case, I don't think there's anything to be done here. Thanks @lzutao.

@bjorn3
Copy link
Member

bjorn3 commented Sep 24, 2020

Every module contributes one codegen unit (before cgu merging). By splitting a module you get more codegen units. This can have an effect on performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants