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

Improve ptr_rotate performance, tests, and benches #61937

Merged
merged 1 commit into from
Aug 9, 2019

Conversation

AaronKutch
Copy link
Contributor

The corresponding issue is #61784. I am not actually sure if miri can handle the test, but I can change the commit if necessary.

@rust-highfive
Copy link
Collaborator

r? @rkruppe

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 18, 2019
@hanna-kruppe
Copy link
Contributor

Sorry, it seems like I can't get around to reviewing this quickly. Maybe @scottmcm can take a look?

@Mark-Simulacrum
Copy link
Member

Re-assigning to @scottmcm but I suspect they might also be unavailable.

cc @rust-lang/libs

@bors
Copy link
Contributor

bors commented Jul 25, 2019

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

@AaronKutch
Copy link
Contributor Author

Is this the right way to resolve a merge conflict? All I did was click the in browser resolve button and fix the problem. The in browser commits look fine, but my local rust branch is freaking out and git pull --force makes my branch two commits ahead of origin, but

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

        modified:   src/doc/book (new commits)
        modified:   src/doc/edition-guide (new commits)
...
        modified:   src/tools/rustfmt (new commits)

Untracked files:
  (use "git add <file>..." to include in what will be committed)

        src/stdsimd/

What is going on here? I want to squash the conflict fix into the original commit, without needing to redownload a rust clone.

@tesuji
Copy link
Contributor

tesuji commented Jul 26, 2019

I think rust prefer rebasing feature branches on top of master in case of merge conflict.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed (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.
2019-07-26T01:08:31.3810554Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-07-26T01:08:31.4008157Z ##[command]git config gc.auto 0
2019-07-26T01:08:31.4095818Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-07-26T01:08:31.4146211Z ##[command]git config --get-all http.proxy
2019-07-26T01:08:31.4292868Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/61937/merge:refs/remotes/pull/61937/merge
---
2019-07-26T01:09:06.2582004Z do so (now or later) by using -b with the checkout command again. Example:
2019-07-26T01:09:06.2582048Z 
2019-07-26T01:09:06.2582309Z   git checkout -b <new-branch-name>
2019-07-26T01:09:06.2582348Z 
2019-07-26T01:09:06.2582429Z HEAD is now at 080f4b6ff Merge ec5523b0028dc93eb10b914f08368779655902d9 into 890881f8f4c77e8670d4b32104c0325fcfefc90f
2019-07-26T01:09:06.2738074Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-07-26T01:09:06.2741338Z ==============================================================================
2019-07-26T01:09:06.2741403Z Task         : Bash
2019-07-26T01:09:06.2741472Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-07-26T01:15:17.0878392Z    Compiling serde_json v1.0.40
2019-07-26T01:15:21.3105105Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-07-26T01:15:29.8907431Z     Finished release [optimized] target(s) in 1m 28s
2019-07-26T01:15:29.8993340Z tidy check
2019-07-26T01:15:30.0998107Z tidy error: /checkout/src/libcore/slice/rotate.rs: missing trailing newline
2019-07-26T01:15:31.6972789Z some tidy checks failed
2019-07-26T01:15:31.6977019Z 
2019-07-26T01:15:31.6977019Z 
2019-07-26T01:15:31.6978817Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-07-26T01:15:31.6982446Z 
2019-07-26T01:15:31.6982719Z 
2019-07-26T01:15:31.6983124Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-07-26T01:15:31.6983428Z Build completed unsuccessfully in 0:01:31
2019-07-26T01:15:31.6983428Z Build completed unsuccessfully in 0:01:31
2019-07-26T01:15:33.0503816Z ##[error]Bash exited with code '1'.
2019-07-26T01:15:33.0544224Z ##[section]Starting: Checkout
2019-07-26T01:15:33.0546258Z ==============================================================================
2019-07-26T01:15:33.0546312Z Task         : Get sources
2019-07-26T01:15:33.0546379Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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 @TimNN. (Feature Requests)

@AaronKutch
Copy link
Contributor Author

I downloaded a fresh depth 1 rust master and used that for this PR. It looks like everything is going well, except that my Github website rust master page is "1 commit ahead, 59 commits behind" for some reason.

Copy link
Member

@scottmcm scottmcm left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here. This approach seems good, though I couldn't repro any material perf difference on my old SandyBridge.

Please respond to the comments I left (with code changes or with prose) and I'll take another look.

src/libcore/slice/rotate.rs Outdated Show resolved Hide resolved
src/libcore/slice/rotate.rs Show resolved Hide resolved
src/libcore/slice/rotate.rs Show resolved Hide resolved
@scottmcm scottmcm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 28, 2019
@AaronKutch
Copy link
Contributor Author

Note that some of the benchmarks are behaving weirdly. I figured out that if I run the x1024s32 benches by themselves, then they consistently get 181 ns plus or minus 2. If I run with all the benchmarks ahead of them, then my algorithm usually gets a 239 ns, while the old one stays at 181 ns. This continues even if I reorder them. The rgb and u8 benches are also weird. Most of the other benches are consistent.

@edmilsonefs
Copy link

Hey! This is a ping from triage, we would like to know if you @scottmcm could give us a few minutes to share your thoughts on last response sent by @AaronKutch .

Thanks.

@rustbot modify labels to -S-waiting-on-author, +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 6, 2019
@scottmcm scottmcm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2019
@AaronKutch
Copy link
Contributor Author

I made some changes to the commit.

@scottmcm scottmcm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 9, 2019
@scottmcm
Copy link
Member

scottmcm commented Aug 9, 2019

Thanks, looks like it should be good now!

@bors r+

@bors
Copy link
Contributor

bors commented Aug 9, 2019

📌 Commit ad7fdb6 has been approved by scottmcm

@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 9, 2019
Centril added a commit to Centril/rust that referenced this pull request Aug 9, 2019
Improve `ptr_rotate` performance, tests, and benches

The corresponding issue is rust-lang#61784. I am not actually sure if miri can handle the test, but I can change the commit if necessary.
@bors
Copy link
Contributor

bors commented Aug 9, 2019

⌛ Testing commit ad7fdb6 with merge 186011a0de31be69649a1fe9f66fed93125b3ca4...

@Centril
Copy link
Contributor

Centril commented Aug 9, 2019

@bors retry rolled up.

bors added a commit that referenced this pull request Aug 9, 2019
Rollup of 4 pull requests

Successful merges:

 - #61937 (Improve `ptr_rotate` performance, tests, and benches)
 - #63302 (Update LLVM submodule)
 - #63337 (Tweak mismatched types error)
 - #63397 (Add tests for some ICEs)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Aug 9, 2019

⌛ Testing commit ad7fdb6 with merge d8f8be4...

bors added a commit that referenced this pull request Aug 9, 2019
Improve `ptr_rotate` performance, tests, and benches

The corresponding issue is #61784. I am not actually sure if miri can handle the test, but I can change the commit if necessary.
@bors
Copy link
Contributor

bors commented Aug 9, 2019

☀️ Test successful - checks-azure
Approved by: scottmcm
Pushing d8f8be4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 9, 2019
@bors bors merged commit ad7fdb6 into rust-lang:master Aug 9, 2019
#[test]
fn brute_force_rotate_test_1() {
// `ptr_rotate` covers so many kinds of pointer usage, that this is just a good test for
// pointers in general. This uses a `[usize; 4]` to hit all algorithms without overwhelming miri
Copy link
Member

Choose a reason for hiding this comment

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

Thanks you so much for keeping Miri in mind here. <3

@@ -1130,6 +1130,44 @@ fn test_rotate_right() {
}
}

#[test]
#[cfg(not(miri))]
Copy link
Member

Choose a reason for hiding this comment

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

These should come with a comment explaining why. I will add a Miri is too slow in one of the Miri-adjustment PRs I have in flight anyway.

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.

None yet