Improve tests and benchmarks for slice::sort and slice::sort_unstable #42882

ghost · 2017-06-24T15:17:27Z

This PR just hardens the tests and improves benchmarks.
More specifically:

Benchmarks don't generate vectors in Bencher::iter loops, but simply clone pregenerated vectors.
Benchmark *_strings doesn't allocate Strings in Bencher::iter loops, but merely clones a Vec<&str>.
Benchmarks use seeded XorShiftRng to be more consistent.
Additional tests for slice::sort are added, which test sorting on slices with several ascending/descending runs. The implementation identifies such runs so it's a good idea to test that scenario a bit.
More checks are added to run-pass/vector-sort-panic-safe.rs. Sort algorithms copy elements around a lot (merge sort uses an auxilliary buffer and pdqsort copies the pivot onto the stack before partitioning, then writes it back into the slice). If elements that are being sorted are internally mutable and comparison function mutates them, it is important to make sure that sort algorithms always use the latest "versions" of elements. New checks verify that this is true for both slice::sort and slice::sort_unstable.

As a side note, all of those improvements were made as part of the parallel sorts PR in Rayon (nikomatsakis/rayon#379) and now I'm backporting them into libcore/libstd.

r? @alexcrichton

alexcrichton · 2017-06-24T16:55:34Z

@bors: r+

Thanks!

bors · 2017-06-24T16:55:34Z

📌 Commit 12205f1 has been approved by alexcrichton

bors · 2017-06-24T17:08:05Z

⌛ Testing commit 12205f1 with merge 32091e1b39664ecd81d82276c1ce000e8a5c75b2...

bors · 2017-06-24T18:20:08Z

💔 Test failed - status-travis

alexcrichton · 2017-06-24T18:56:10Z

Looks like a legitimate failure?

[01:06:00] 
[01:06:00] failures:
[01:06:00] 
[01:06:00] ---- [run-pass] run-pass/vector-sort-panic-safe.rs stdout ----
[01:06:00] 	
[01:06:00] error: test run failed!
[01:06:00] status: exit code: 101
[01:06:00] command: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release/remote-test-client run /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/vector-sort-panic-safe.stage2-arm-unknown-linux-gnueabihf
[01:06:00] stdout:
[01:06:00] ------------------------------------------
[01:06:00] uploaded "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/vector-sort-panic-safe.stage2-arm-unknown-linux-gnueabihf", waiting for result
[01:06:00] 
[01:06:00] ------------------------------------------
[01:06:00] stderr:
[01:06:00] ------------------------------------------
[01:06:00] thread 'main' panicked at 'attempt to calculate the remainder with a divisor of zero', /checkout/src/test/run-pass/vector-sort-panic-safe.rs:151
[01:06:00] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:06:00] 
[01:06:00] ------------------------------------------
[01:06:00] 
[01:06:00] thread '[run-pass] run-pass/vector-sort-panic-safe.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2480
[01:06:00] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:06:00] 
[01:06:00] 
[01:06:00] failures:
[01:06:00]     [run-pass] run-pass/vector-sort-panic-safe.rs
[01:06:00] 
[01:06:00] test result: �[31mFAILED�(B�[m. 2696 passed; 1 failed; 9 ignored; 0 measured; 0 filtered out

ghost · 2017-06-25T21:16:27Z

The failure is apparently due to a codegen bug on ARM platforms.

I believe simply moving let mut rng = thread_rng() (here) outside the innermost loop would work around the build failure. Moving that line of code is a good idea anyway, so... can we do that or should we wait until the codegen issue is resolved?

alexcrichton · 2017-06-26T01:22:21Z

@stjepang yeah sounds good to me!

ghost · 2017-06-26T09:21:05Z

All right, let's see if it works this time...

alexcrichton · 2017-06-26T11:24:37Z

@bors: r+

bors · 2017-06-26T11:24:37Z

📌 Commit 723833f has been approved by alexcrichton

ghost · 2017-06-26T18:03:33Z

The new failures are spurious, it seems... three targets were not tested at all (they're marked with ! rather than X). Perhaps retry?

kennytm · 2017-06-26T18:56:42Z

@stjepang Don't worry about the travis-ci/pr failure, it is fine as long as the IMAGE=x86_64-gnu-llvm-3.7 ALLOW_PR=1 RUST_BACKTRACE=1 one passes.

Mark-Simulacrum · 2017-06-28T17:23:05Z

@bors rollup

@alexcrichton

…ches, r=alexcrichton Improve tests and benchmarks for slice::sort and slice::sort_unstable This PR just hardens the tests and improves benchmarks. More specifically: 1. Benchmarks don't generate vectors in `Bencher::iter` loops, but simply clone pregenerated vectors. 2. Benchmark `*_strings` doesn't allocate Strings in `Bencher::iter` loops, but merely clones a `Vec<&str>`. 3. Benchmarks use seeded `XorShiftRng` to be more consistent. 4. Additional tests for `slice::sort` are added, which test sorting on slices with several ascending/descending runs. The implementation identifies such runs so it's a good idea to test that scenario a bit. 5. More checks are added to `run-pass/vector-sort-panic-safe.rs`. Sort algorithms copy elements around a lot (merge sort uses an auxilliary buffer and pdqsort copies the pivot onto the stack before partitioning, then writes it back into the slice). If elements that are being sorted are internally mutable and comparison function mutates them, it is important to make sure that sort algorithms always use the latest "versions" of elements. New checks verify that this is true for both `slice::sort` and `slice::sort_unstable`. As a side note, all of those improvements were made as part of the parallel sorts PR in Rayon (nikomatsakis/rayon#379) and now I'm backporting them into libcore/libstd. r? @alexcrichton

Mark-Simulacrum · 2017-06-28T21:03:00Z

@bors r- This failed the rollup, with I think the same failure. Possibly will be fixed after the LLVM update, not sure.

[01:05:40] ---- [run-pass] run-pass/vector-sort-panic-safe.rs stdout ----
[01:05:40]
[01:05:40] error: test run failed!
[01:05:40] status: exit code: 101
[01:05:40] command: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release/remote-test-client run /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/vector-sort-panic-safe.stage2-arm-unknown-linux-gnueabihftdout:
[01:05:40] ------------------------------------------
[01:05:40] uploaded "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/vector-sort-panic-safe.stage2-arm-unknown-linux-gnueabihf", waiting for result
[01:05:40]
[01:05:40] ------------------------------------------
[01:05:40] stderr:
[01:05:40] ------------------------------------------
[01:05:40] thread 'main' panicked at 'attempt to calculate the remainder with a divisor of zero', /checkout/src/test/run-pass/vector-sort-panic-safe.rs:152
[01:05:40] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:05:40]
[01:05:40] ------------------------------------------
[01:05:40]
[01:05:40] thread '[run-pass] run-pass/vector-sort-panic-safe.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2479
[01:05:40] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:05:40]
[01:05:40]
[01:05:40] failures:
[01:05:40]     [run-pass] run-pass/vector-sort-panic-safe.rs
[01:05:40]
[01:05:40] test result: FAILED. 2697 passed; 1 failed; 9 ignored; 0 measured; 0 filtered out
[01:05:40]
[01:05:40] thread 'main' panicked at 'Some tests failed', /checkout/src/tools/compiletest/src/main.rs:325

ghost · 2017-06-28T22:23:02Z

It will be fixed after the update, yes.

ghost · 2017-06-30T10:09:47Z

Ok, LLVM has been updated. Should I rebase now?

alexcrichton · 2017-06-30T22:33:48Z

@bors: r+

Nah should still apply cleanly!

bors · 2017-06-30T22:33:49Z

📌 Commit 723833f has been approved by alexcrichton

bors · 2017-07-01T13:31:38Z

⌛ Testing commit 723833f with merge 05b5797...

@alexcrichton

…xcrichton Improve tests and benchmarks for slice::sort and slice::sort_unstable This PR just hardens the tests and improves benchmarks. More specifically: 1. Benchmarks don't generate vectors in `Bencher::iter` loops, but simply clone pregenerated vectors. 2. Benchmark `*_strings` doesn't allocate Strings in `Bencher::iter` loops, but merely clones a `Vec<&str>`. 3. Benchmarks use seeded `XorShiftRng` to be more consistent. 4. Additional tests for `slice::sort` are added, which test sorting on slices with several ascending/descending runs. The implementation identifies such runs so it's a good idea to test that scenario a bit. 5. More checks are added to `run-pass/vector-sort-panic-safe.rs`. Sort algorithms copy elements around a lot (merge sort uses an auxilliary buffer and pdqsort copies the pivot onto the stack before partitioning, then writes it back into the slice). If elements that are being sorted are internally mutable and comparison function mutates them, it is important to make sure that sort algorithms always use the latest "versions" of elements. New checks verify that this is true for both `slice::sort` and `slice::sort_unstable`. As a side note, all of those improvements were made as part of the parallel sorts PR in Rayon (nikomatsakis/rayon#379) and now I'm backporting them into libcore/libstd. r? @alexcrichton

bors · 2017-07-01T15:52:39Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 05b5797 to master...

Improve sort tests and benchmarks

12205f1

rust-highfive assigned alexcrichton Jun 24, 2017

Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 24, 2017

ghost mentioned this pull request Jun 25, 2017

Bad codegen (most probably) on ARMv7 #42893

Closed

Move thread_rng() outside the loop

723833f

arielb1 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 27, 2017

Mark-Simulacrum mentioned this pull request Jun 28, 2017

Rollup of 7 pull requests #42952

Closed

bors merged commit 723833f into rust-lang:master Jul 1, 2017

ghost deleted the improve-sort-tests-and-benches branch July 1, 2017 20:30

japaric mentioned this pull request Jul 4, 2017

Serious regression in binary size when using LTO #43054

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve tests and benchmarks for slice::sort and slice::sort_unstable #42882

Improve tests and benchmarks for slice::sort and slice::sort_unstable #42882

ghost commented Jun 24, 2017

alexcrichton commented Jun 24, 2017

bors commented Jun 24, 2017

bors commented Jun 24, 2017

bors commented Jun 24, 2017

alexcrichton commented Jun 24, 2017

ghost commented Jun 25, 2017

alexcrichton commented Jun 26, 2017

ghost commented Jun 26, 2017

alexcrichton commented Jun 26, 2017

bors commented Jun 26, 2017

ghost commented Jun 26, 2017

kennytm commented Jun 26, 2017

Mark-Simulacrum commented Jun 28, 2017

Mark-Simulacrum commented Jun 28, 2017

ghost commented Jun 28, 2017

ghost commented Jun 30, 2017

alexcrichton commented Jun 30, 2017

bors commented Jun 30, 2017

bors commented Jul 1, 2017

bors commented Jul 1, 2017

Improve tests and benchmarks for slice::sort and slice::sort_unstable #42882

Improve tests and benchmarks for slice::sort and slice::sort_unstable #42882

Conversation

ghost commented Jun 24, 2017

alexcrichton commented Jun 24, 2017

bors commented Jun 24, 2017

bors commented Jun 24, 2017

bors commented Jun 24, 2017

alexcrichton commented Jun 24, 2017

ghost commented Jun 25, 2017

alexcrichton commented Jun 26, 2017

ghost commented Jun 26, 2017

alexcrichton commented Jun 26, 2017

bors commented Jun 26, 2017

ghost commented Jun 26, 2017

kennytm commented Jun 26, 2017

Mark-Simulacrum commented Jun 28, 2017

Mark-Simulacrum commented Jun 28, 2017

ghost commented Jun 28, 2017

ghost commented Jun 30, 2017

alexcrichton commented Jun 30, 2017

bors commented Jun 30, 2017

bors commented Jul 1, 2017

bors commented Jul 1, 2017