Skip to content

Update fannkuchredux benchmark #16999

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

Merged
merged 2 commits into from
Sep 7, 2014
Merged

Update fannkuchredux benchmark #16999

merged 2 commits into from
Sep 7, 2014

Conversation

brson
Copy link
Contributor

@brson brson commented Sep 5, 2014

From the discussion on reddit:
http://www.reddit.com/r/rust/comments/2fenlg/benchmark_improvement_fannkuchredux/

This adds two variants: the primary, that uses an unsafe block, and a secondary
that is completely safe.

The one with the unsafe block matches clang's performance and beats gcc's.

From the discussion on reddit:
http://www.reddit.com/r/rust/comments/2fenlg/benchmark_improvement_fannkuchredux/

This adds two variants: the primary, that uses an unsafe block, and a secondary
that is completely safe.

The one with the unsafe block matches clang's performance and beats gcc's.
@huonw
Copy link
Member

huonw commented Sep 5, 2014

Hm, is adjusting slice::reverse to use the unsafe code for performance acceptable? (This should leave the -safe one as fast as the other.)

@brson
Copy link
Contributor Author

brson commented Sep 5, 2014

@huonw I'm testing that change to slice::reverse now.

@brson
Copy link
Contributor Author

brson commented Sep 5, 2014

@huonw Optimized reverse so that the safe fannkuchredux matches C++ perf!

@alexcrichton
Copy link
Member

It'd be awesome if we would show off idiomatic rust code beating/matching C++ rather than straight ports of the C++ benchmarks, and I'm curious, with the reverse improvements, is the current rust version that much slower?

@huonw
Copy link
Member

huonw commented Sep 5, 2014

http://www.reddit.com/r/rust/comments/2fenlg/benchmark_improvement_fannkuchredux/ck8swcx?context=1

Iirc the 'original' listed there uses @dotdash's reverse (the fast one), but I can't remember exactly which cfg I used. If so, it is still slower. (I imagine that removing the heap allocation from the original Rust one would give a speed up, but that would basically be the same as the C++ anyway.)

@dotdash
Copy link
Contributor

dotdash commented Sep 5, 2014

result -> reverse in the second commit message. That had me quite confused for a moment.

@huonw
Copy link
Member

huonw commented Sep 5, 2014

Ah, no, I think that 'original' was the truly original code (makes sense...), but just substituting in the faster reverse takes 1.55s (vs. 1.16 for the fastest Rust).

unsafe {
let pa: *mut T = self.unsafe_mut_ref(i);
let pb: *mut T = self.unsafe_mut_ref(ln - i - 1);
ptr::swap(pa, pb);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this just be mem::swap instead? That way there's no raw ptrs.

@arthurprs
Copy link
Contributor

Shouldn't we prefer the neater alternative, also by @brson

Edit: I don't think we need to leave a note saying it improves a benchmark. It's a valid optimization as observed in other functions as well.

    let p = self.as_mut_ptr();
    unsafe {
        for off in range(0, k / 2) {
            std::ptr::swap(p.offset(off), p.offset(k - 1 - off));
        }
    }

@brson
Copy link
Contributor Author

brson commented Sep 5, 2014

I'm not clear on what people are suggesting happens here.

@brson
Copy link
Contributor Author

brson commented Sep 5, 2014

Updated the description of the second commit.

This makes the completely safe implementation of fannkuchredux perform
the same as C++. Yay, Rust.
@brson
Copy link
Contributor Author

brson commented Sep 5, 2014

I removed the comment about fankuch. mem::swap can't be used here because of borrowing conflicts.

@huonw
Copy link
Member

huonw commented Sep 5, 2014

I would r+ but I wrote the code, basically, so that feels bad. :)

bors added a commit that referenced this pull request Sep 7, 2014
From the discussion on reddit:
http://www.reddit.com/r/rust/comments/2fenlg/benchmark_improvement_fannkuchredux/

This adds two variants: the primary, that uses an unsafe block, and a secondary
that is completely safe.

The one with the unsafe block matches clang's performance and beats gcc's.
@bors bors closed this Sep 7, 2014
@bors bors merged commit 7e12e67 into rust-lang:master Sep 7, 2014
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.

7 participants