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

Mention [T]::sort is stable in docs #29579

Merged
merged 1 commit into from
Nov 5, 2015
Merged

Conversation

steveklabnik
Copy link
Member

Fixes #27322

@rust-highfive
Copy link
Collaborator

r? @brson

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

@apasel422
Copy link
Contributor

@steveklabnik Should this expound on stability or link to an external definition of it (e.g. https://en.wikipedia.org/wiki/Sorting_algorithm#Stability)? Or is that overkill?

@steveklabnik
Copy link
Member Author

@apasel422 for now, it's overkill. I have metabugs to overhaul all of these docs, I'll be doing that kind of extra work at that point. It's easier when you're doing big chunks at one time.

@apasel422
Copy link
Contributor

@bors: r+ a118aa2 rollup

@nagisa
Copy link
Member

nagisa commented Nov 4, 2015

I think stability of sort algorithm is a well known concept that needs no introduction.

@alexcrichton
Copy link
Member

@bors: r-

Ah if ok I'd like to run this by the libs team, this is a pretty big guarantee to provide and I know we've talked about this in the past one way or another. I forget the exact outcome, but I vaguely remember us explicitly wanting to not guarantee it's not stable, but now I may also be misremembering!

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 4, 2015
@steveklabnik
Copy link
Member Author

Yeah, given the discussion, it looked like it already was. No worries on waiting though, would rather have it double checked than not!

@Gankra
Copy link
Contributor

Gankra commented Nov 5, 2015

I distinctly remember us intending to guarantee stability. Otherwise we should be using quicksort with some decent rng (the n^2 behaviour is inconceivable with proper rng).

@huonw
Copy link
Member

huonw commented Nov 5, 2015

FWIW, sort_by is documented as stable and sort is documented as being equivalent to a certain call to sort_by, so sort is already transitively documented as stable.

Otherwise we should be using quicksort with some decent rng (the n^2 behaviour is inconceivable with proper rng).

Having an RNG/global state (it would presumably need thread locals, and maybe even seeding from some source?) would probably make the sort inappropriate for low-level use-cases, and it'd be great if sorts worked in as many places as possible.

@Gankra
Copy link
Contributor

Gankra commented Nov 5, 2015

Maybe I'm misunderstanding, but why use thread locals? Just get a new seed from the OS or whatever on every call to sort that isn't small enough to warrant insertionsort. Since it's already an incredibly expensive op, that would be a drop in the bucket, I think?

But yes, a libcore-friendly sort would be best (wikisort, I guess?)

@alexcrichton
Copy link
Member

Ah, it appears I was misremembering! Thanks for the clarificatins @gankro and @huonw

@bors: r=apasel422 a118aa2

@huonw
Copy link
Member

huonw commented Nov 5, 2015

Since it's already an incredibly expensive op, that would be a drop in the bucket, I think?

Sorting isn't that expensive. It's certainly going to be a drop in the bucket for a very long sort, but it won't be so small for moderate lengths, which can totally happen in tight loops (e.g. a whole pile of vectors with length less than 1000 that each need to be sorted).

#![feature(test, read_exact)]
extern crate rand;
extern crate test;

use std::io::prelude::*;
use std::fs;
use rand::Rng;

#[bench]
fn os_rng_u32(b: &mut test::Bencher) {
    b.iter(|| {
        rand::OsRng::new().unwrap().gen::<u32>();
    })
}

#[bench]
fn read_dev_urandom_u32(b: &mut test::Bencher) {
    b.iter(|| {
        let mut b = [0; 4];
        fs::File::open("/dev/urandom").unwrap().read_exact(&mut b).unwrap();
    })
}

#[bench]
fn sort_0050(b: &mut test::Bencher) {
    let v = rand::thread_rng().gen_iter::<u32>().take(50).collect::<Vec<_>>();
    b.iter(|| {
        v.clone().sort();
    })
}
#[bench]
fn sort_0100(b: &mut test::Bencher) {
    let v = rand::thread_rng().gen_iter::<u32>().take(100).collect::<Vec<_>>();
    b.iter(|| {
        v.clone().sort();
    })
}
#[bench]
fn sort_0500(b: &mut test::Bencher) {
    let v = rand::thread_rng().gen_iter::<u32>().take(500).collect::<Vec<_>>();
    b.iter(|| {
        v.clone().sort();
    })
}

#[bench]
fn sort_1000(b: &mut test::Bencher) {
    let v = rand::thread_rng().gen_iter::<u32>().take(1000).collect::<Vec<_>>();
    b.iter(|| {
        v.clone().sort();
    })
}
test os_rng_u32           ... bench:         731 ns/iter (+/- 39)
test read_dev_urandom_u32 ... bench:       1,789 ns/iter (+/- 116)
test sort_0050            ... bench:         552 ns/iter (+/- 44)
test sort_0100            ... bench:       1,417 ns/iter (+/- 102)
test sort_0500            ... bench:      10,675 ns/iter (+/- 430)
test sort_1000            ... bench:      27,348 ns/iter (+/- 771)

Also, random functions doing "random" (hah) syscalls is unfortunate from a sandboxing point of view. (I guess ones related to randomness are probably going to generally have to be whitelisted anyway, though, just like ones for allocation.)

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Nov 5, 2015
steveklabnik added a commit to steveklabnik/rust that referenced this pull request Nov 5, 2015
bors added a commit that referenced this pull request Nov 5, 2015
@bors bors merged commit a118aa2 into rust-lang:master Nov 5, 2015
@steveklabnik steveklabnik deleted the gh27322 branch June 19, 2016 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

9 participants