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

DO NOT MERGE: map_split perf test #51643

Closed
wants to merge 3 commits into from
Closed

DO NOT MERGE: map_split perf test #51643

wants to merge 3 commits into from

Conversation

joshlf
Copy link
Contributor

@joshlf joshlf commented Jun 19, 2018

This PR represents the cherry-pick of #51466 and #51630 onto b81da2 (the commit just before #51466). Comparing the performance of this PR to the performance of b81da2 should give us an apples-to-apples comparison of the optimized version of map_split against a previous world in which map_split doesn't exist, and refcounts can only represent a single writing reference.

cc @nnethercote @rkruppe @RalfJung @kennytm

bors and others added 2 commits June 19, 2018 15:03
@bors @joshlf
Auto merge of rust-lang#51466 - joshlf:ref-split, r=dtolnay
498e9ba 
Add Ref/RefMut map_split method

As proposed [here](https://internals.rust-lang.org/t/make-refcell-support-slice-splitting/7707).

TLDR: Add a `map_split` method that allows multiple `RefMut`s to exist simultaneously so long as they refer to non-overlapping regions of the original `RefCell`. This is useful for things like the slice `split_at_mut` method.
@joshlf
Optimize RefCell refcount tracking
8f0fce2
@rust-highfive
Copy link
Collaborator

r? @aidanhs

(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 19, 2018
@joshlf joshlf changed the title DO NOT MERGE: Map split perf test DO NOT MERGE: map_split perf test Jun 19, 2018
@nnethercote nnethercote mentioned this pull request Jun 19, 2018
Merged
@Mark-Simulacrum
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Jun 19, 2018

🔒 Merge conflict

@joshlf
Copy link
Contributor Author

joshlf commented Jun 20, 2018

OK, I resolved the merge conflicts, so bors should be OK with it now.

@Mark-Simulacrum
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Jun 20, 2018

⌛ Trying commit 2d61e72 with merge d8dd3b5...

bors added a commit that referenced this pull request Jun 20, 2018
@bors
Auto merge of #51643 - joshlf:map-split-perf-test, r=<try>
d8dd3b5 
DO NOT MERGE: map_split perf test

This PR represents the cherry-pick of #51466 and #51630 onto [b81da2](b81da27) (the commit just before #51466). Comparing the performance of this PR to the performance of b81da2 should give us an apples-to-apples comparison of the optimized version of `map_split` against a previous world in which `map_split` doesn't exist, and refcounts can only represent a single writing reference.

cc @nnethercote @rkruppe @RalfJung @kennytm
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (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. 
[01:00:26] ....................................................................................................
[01:00:32] ....................................................................................................
[01:00:40] ....................................................................................................
[01:00:45] ...i................................................................................................
[01:00:52] ..i..ii...................................................................................FFF.F.....
[01:01:06] ....................................................................................................
[01:01:09] .........................................................test [compile-fail] compile-fail/issue-22638.rs has been running for over 60 seconds
[01:01:12] ...........................i...............
[01:01:16] ..............................i.....................................................................
---
[01:01:32] failures:
[01:01:32] 
[01:01:32] ---- [compile-fail] compile-fail/not-panic-safe-2.rs stdout ----
[01:01:32] 
[01:01:32] error: /checkout/src/test/compile-fail/not-panic-safe-2.rs:20: unexpected error: '20:5: 20:31: the trait bound `std::cell::UnsafeCell<isize>: std::panic::RefUnwindSafe` is not satisfied in `std::cell::RefCell<i32>` [E0277]'
[01:01:32] 
[01:01:32] error: /checkout/src/test/compile-fail/not-panic-safe-2.rs:20: expected error not found: `std::cell::UnsafeCell<usize>: std::panic::RefUnwindSafe` is not satisfied
[01:01:32] 
[01:01:32] error: 1 unexpected errors found, 1 expected errors not found
[01:01:32] status: exit code: 101
[01:01:32] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/compile-fail/not-panic-safe-2.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail/not-panic-safe-2/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail/not-panic-safe-2/auxiliary" "-A" "unused"
[01:01:32] unexpected errors (from JSON output): [
[01:01:32]     Error {
[01:01:32]         line_num: 20,
[01:01:32]         kind: Some(
[01:01:32]         ),
[01:01:32]         ),
[01:01:32]         msg: "20:5: 20:31: the trait bound `std::cell::UnsafeCell<isize>: std::panic::RefUnwindSafe` is not satisfied in `std::cell::RefCell<i32>` [E0277]"
[01:01:32] ]
[01:01:32] 
[01:01:32] not found errors (from test file): [
[01:01:32]     Error {
[01:01:32]     Error {
[01:01:32]         line_num: 20,
[01:01:32]         kind: Some(
[01:01:32]             Error
[01:01:32]         ),
[01:01:32]         msg: "`std::cell::UnsafeCell<usize>: std::panic::RefUnwindSafe` is not satisfied"
[01:01:32] ]
[01:01:32] 
[01:01:32] thread '[compile-fail] compile-fail/not-panic-safe-2.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:1284:13
[01:01:32] 
[01:01:32] 
[01:01:32] ---- [compile-fail] compile-fail/not-panic-safe-3.rs stdout ----
[01:01:32] 
[01:01:32] error: /checkout/src/test/compile-fail/not-panic-safe-3.rs:20: unexpected error: '20:5: 20:32: the trait bound `std::cell::UnsafeCell<isize>: std::panic::RefUnwindSafe` is not satisfied in `std::cell::RefCell<i32>` [E0277]'
[01:01:32] 
[01:01:32] error: /checkout/src/test/compile-fail/not-panic-safe-3.rs:20: expected error not found: `std::cell::UnsafeCell<usize>: std::panic::RefUnwindSafe` is not satisfied
[01:01:32] 
[01:01:32] error: 1 unexpected errors found, 1 expected errors not found
[01:01:32] status: exit code: 101
[01:01:32] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/compile-fail/not-panic-safe-3.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail/not-panic-safe-3/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail/not-panic-safe-3/auxiliary" "-A" "unused"
[01:01:32] unexpected errors (from JSON output): [
[01:01:32]     Error {
[01:01:32]         line_num: 20,
[01:01:32]         kind: Some(
[01:01:32]         ),
[01:01:32]         ),
[01:01:32]         msg: "20:5: 20:32: the trait bound `std::cell::UnsafeCell<isize>: std::panic::RefUnwindSafe` is not satisfied in `std::cell::RefCell<i32>` [E0277]"
[01:01:32] ]
[01:01:32] 
[01:01:32] not found errors (from test file): [
[01:01:32]     Error {
[01:01:32]     Error {
[01:01:32]         line_num: 20,
[01:01:32]         kind: Some(
[01:01:32]             Error
[01:01:32]         ),
[01:01:32]         msg: "`std::cell::UnsafeCell<usize>: std::panic::RefUnwindSafe` is not satisfied"
[01:01:32] ]
[01:01:32] 
[01:01:32] thread '[compile-fail] compile-fail/not-panic-safe-3.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:1284:13
[01:01:32] 
[01:01:32] 
[01:01:32] ---- [compile-fail] compile-fail/not-panic-safe-4.rs stdout ----
[01:01:32] 
[01:01:32] error: /checkout/src/test/compile-fail/not-panic-safe-4.rs:19: unexpected error: '19:5: 19:28: the trait bound `std::cell::UnsafeCell<isize>: std::panic::RefUnwindSafe` is not satisfied in `std::cell::RefCell<i32>` [E0277]'
[01:01:32] 
[01:01:32] error: /checkout/src/test/compile-fail/not-panic-safe-4.rs:19: expectt file): [
[01:01:32]     Error {
[01:01:32]         line_num: 19,
[01:01:32]         kind: Some(
[01:01:32]         ),
[01:01:32]         ),
[01:01:32]         msg: "`std::cell::UnsafeCell<usize>: std::panic::RefUnwindSafe` is not satisfied"
[01:01:32] ]
[01:01:32] 
[01:01:32] thread '[compile-fail] compile-fail/not-panic-safe-6.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:1284:13
[01:01:32] 
---
[01:01:32] 
[01:01:32] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:498:22
[01:01:32] 
[01:01:32] 
[01:01:32] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/compile-fail" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/compile-fail" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "compile-fail" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-3.9/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "3.9.1\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:01:32] 
[01:01:32] 
[01:01:32] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:01:32] Build completed unsuccessfully in 0:14:55
[01:01:32] Build completed unsuccessfully in 0:14:55
[01:01:32] Makefile:58: recipe for target 'check' failed
[01:01:32] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:092e51aa
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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)

@bors
Copy link
Contributor

bors commented Jun 20, 2018

☀️ Test successful - status-travis
State: approved= try=True

@RalfJung
Copy link
Member

Comparing the performance of this PR to the performance of b81da2 should give us an apples-to-apples comparison of the optimized version of map_split against a previous world in which map_split doesn't exist, and refcounts can only represent a single writing reference.

It seems that bors merges master into this before running perf? So to actually compare apples with apples we'd have to take care that the two merges happen at the same time?

@joshlf
Copy link
Contributor Author

joshlf commented Jun 20, 2018

It seems that bors merges master into this before running perf? So to actually compare apples with apples we'd have to take care that the two merges happen at the same time?

I'm not sure what you mean by "two merges happen at the same time"?

@RalfJung
Copy link
Member

I mean that if this PR gets merged into master and perf runs, and then 2 days later the other PR gets merged into master and perf runs, the difference could also come from stuff that has happened in master in the mean time.

Ideally, perf would run on the PR, not on the PR merged into master. Or maybe it does? I just inferred it would not from the fact that merge conflicts were an issue.

@Mark-Simulacrum
Copy link
Member

Perf running for d8dd3b5. @RalfJung is I believe correct that bors try will take the PR head and rebase it atop master before building.

@pietroalbini
Copy link
Member

Ping from triage! @Mark-Simulacrum are the perf results ready?

@kennytm
Copy link
Member

kennytm commented Jun 25, 2018

Perf is in fact ready http://perf.rust-lang.org/compare.html?start=ac8d1f7623f55746ff8f59d6b26f03c39cee5844&end=d8dd3b51bfd1c672a39e4bb912ecfa5b801b09db&stat=instructions%3Au

@joshlf
Copy link
Contributor Author

joshlf commented Jun 25, 2018

@kennytm Do you disagree with the concern that others have raised that that perf run isn't an apples-to-apples comparison?

@kennytm
Copy link
Member

kennytm commented Jun 25, 2018

Honestly I think the goal of the perf check is to show a -4% improvement to offset #51466 (comment), not a strict apple-to-apple comparison. The perf result in #51630 (comment) should be a sufficient signal to say the fix works.

@kennytm
Copy link
Member

kennytm commented Jun 28, 2018 •
edited
Loading

#51630 has been r+'ed (plus we've got another apple-to-apple comparison at #51630 (comment)), so closing this PR now.

@kennytm kennytm closed this Jun 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@aidanhs aidanhs

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@joshlf @rust-highfive @Mark-Simulacrum @bors @RalfJung @pietroalbini @kennytm @aidanhs