Performance has gotten significantly worse #2719

Closed
brson opened this Issue Jun 25, 2012 · 10 comments

Projects

None yet

5 participants

@brson
Collaborator
brson commented Jun 25, 2012

Rustbot shows graph500-bfs, rustc, k-nucleotide, mandelbrot, and others as losing a lot of perf.

graph500-bfs went from a few seconds to thousands of seconds. Rustbot fingers 1d6fb24 as the culprit.

@eholk
eholk commented Jun 25, 2012

It seems like there's a good chance this is related to me moving vector append into the library. I'm making a few tweaks now that should improve performance.

@eholk
eholk commented Jun 25, 2012

Actually, after looking at the graphs some more, it looks like my change isn't responsible for the latest perf regression. That said, there's still a few v += [x] expressions that can be replaced with push(v, x).

@eholk eholk added a commit that referenced this issue Jun 26, 2012
@eholk eholk vec::slice is faster now (Issue #2719) b837f37
@msullivan

Tracked it down. It looks like the libcore vector code was (accidentally) not getting called until my patch landed.

So the regression is due to moving vec append to the library, and the extra copies that causes.

@eholk
eholk commented Jun 26, 2012

I just did a grep for += [ over the whole tree, and there are still a lot of singleton vector appends to track down. This should win back the perf we lost on rustc.

How hard would it be to let us pass self by - or +? This seems like a better long term fix than telling everyone just to use vec::push.

The issue for allowing modes on self is #2585.

@pcwalton
Collaborator

I think dvec is probably the real long-term fix, no?

@eholk eholk added a commit that referenced this issue Jun 26, 2012
@eholk eholk More perf tweaks (issue #2719) a082816
@eholk
eholk commented Jun 26, 2012

@msullivan just pointed out that vec::push_all and vec::append are doing a lot of unnecessary bounds checks that could probably be removed using unsafe code. This could gain some of our speed back.

@eholk eholk added a commit that referenced this issue Jun 27, 2012
@eholk eholk Remove unnecessary bounds checks in vec::push_all (issue #2719)
Don't needlessly drop closures (issue #2603)
133fdc1
@eholk eholk added a commit that referenced this issue Jun 28, 2012
@eholk eholk Replace more vector + (issue #2719) ae06546
@catamorphism

@eholk and @msullivan : is this resolved for now?

@eholk
eholk commented Jun 28, 2012

The perf bots are still showing that we're about 5 seconds behind where we used to be on rustc. There's definitely more work to do, but to me the performance is tolerable again.

I can keep trying to win back those 5 seconds if people don't want to take the hit.

@eholk eholk added a commit that referenced this issue Jun 28, 2012
@eholk eholk replace more vector + (issue #2719) 59221e9
@eholk eholk added a commit that referenced this issue Jun 28, 2012
@eholk eholk Replaced almost all vector+ in rustc (#2719)
Didn't update shape because the changes were causing segfaults.
87eaf91
@eholk
eholk commented Jun 29, 2012

I replaced pretty much all the calls to + with various append, push, push_all, append_one calls, and now rustc is faster than it was before.

@eholk eholk closed this Jun 29, 2012
@brson
Collaborator
brson commented Jun 29, 2012

Nice work @eholk!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment