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

libcollections: move Vec::push slow path out #23670

Merged
merged 1 commit into from
Mar 25, 2015
Merged

libcollections: move Vec::push slow path out #23670

merged 1 commit into from
Mar 25, 2015

Conversation

emberian
Copy link
Member

Makes Vec::push considerably smaller: 25 instructions, rather than 42, on
x86_64.

@rust-highfive
Copy link
Collaborator

r? @aturon

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

@emberian
Copy link
Member Author

r? @pcwalton

@rust-highfive rust-highfive assigned pcwalton and unassigned aturon Mar 24, 2015
@Gankra
Copy link
Contributor

Gankra commented Mar 24, 2015

Vec already has a resize fn

@Gankra
Copy link
Contributor

Gankra commented Mar 24, 2015

See also the private grow_capacity method for some pre-existing refactoring of this functionality.

@emberian
Copy link
Member Author

@gankro using grow_capacity is a non-starter since the capacity calculation (and associated panic) don't want to be in the fast path. I'm not sure how this resize function doesn't conflict with the other one, but I've moved it to an inner helper function now (waiting for build to test if that still works as desired)

Makes Vec::push considerably smaller: 25 instructions, rather than 42, on
x86_64.
@emberian
Copy link
Member Author

Yeah, new version works the same.

@huonw
Copy link
Member

huonw commented Mar 24, 2015

Do you happen to have some before/after benchmarks?

@Gankra
Copy link
Contributor

Gankra commented Mar 24, 2015

@cmr I was moreso suggesting there might be a better refactoring to reduce duplication, but this certainly doesn't make it worse.

@emberian
Copy link
Member Author

@huonw No, but I could fabricate one that is predicated on icache residency.

@emberian
Copy link
Member Author

(@erickt also mentioned wanting this in serde)

@alexcrichton
Copy link
Member

I would personally also prefer to see at least one benchmark showing one way or another here. This is unfortunately a loss in readability in the code and is an idiom we only rarely apply today, so I would prefer to have some numbers backing it up showing why the loss in readability is worth it.

@emberian
Copy link
Member Author

Smaller function size is the main goal (this makes push 40% smaller), which can help inlining in complex functions and reduce icache pressure. Here's one silly benchmark:

extern crate test;

#[bench]
fn x(b: &mut test::Bencher) {
    let mut v = Vec::with_capacity(100);
    b.iter(|| { v.extend(0..100); v.truncate(0); });
}
Before: test x ... bench:       144 ns/iter (+/- 7)
After:  test x ... bench:        81 ns/iter (+/- 4)

@emberian
Copy link
Member Author

For reference, against the C++ equivalent:

#include <vector>
#include <cstdint>

extern "C" {
void cpp_version() {
    static std::vector<int32_t> v(100);
    for (int i = 0; i < 100; i++) {
        v.push_back(i);
    }
    v.resize(0);
}
}
extern crate test;

#[bench]
fn x(b: &mut test::Bencher) {
    let mut v = Vec::with_capacity(100);
    b.iter(|| { v.extend(0..100); v.truncate(0); });
}

#[link(name = "foo")]
#[link(name = "stdc++")]
extern "C" {
    fn cpp_version();
}

#[bench]
fn cpp(b: &mut test::Bencher) {
    b.iter(|| unsafe { cpp_version() });
}
$ clang++ -O2 -fPIC -std=c++11 -c foo.cpp
$ ar rc libfoo.a foo.o
$ LD_LIBRARY_PATH=x86_64-unknown-linux-gnu/stage1/lib ./x86_64-unknown-linux-gnu/stage1/bin/rustc --test -O -L native=. foo.rs
$ ./foo --bench

running 2 tests
test cpp ... bench:       130 ns/iter (+/- 9)
test x   ... bench:        83 ns/iter (+/- 5)

Maybe framing it as "Make Vec::push faster than C++ vector::push_back" makes it go down better ;)

@emberian
Copy link
Member Author

@bors: r=pcwalton rollup

@bors
Copy link
Contributor

bors commented Mar 25, 2015

📌 Commit 0e838f7 has been approved by pcwalton

@bors
Copy link
Contributor

bors commented Mar 25, 2015

⌛ Testing commit 0e838f7 with merge 928e2e2...

bors added a commit that referenced this pull request Mar 25, 2015
Makes Vec::push considerably smaller: 25 instructions, rather than 42, on
x86_64.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants