Closed
Description
According to perf.rust-lang.org graphs the tuple-stress benchmark has regressed 30% in compile time lately, pointing to #43554 as the culprit.
I've downloaded the nightly-2017-08-05
toolchain and the nightly-2017-08-06
toolchain where 08-06 has this PR and 08-05 doesn't. The notable differences in time-passes are:
pass | after | before |
---|---|---|
borrow checking | 2.556 | 0.920 |
const checking | 1.728 | 0.451 |
translation | 0.798 | 0.682 |
expansion | 0.168 | 0.097 |
cc @eddyb
Metadata
Metadata
Assignees
Labels
Type
Projects
Relationships
Development
No branches or pull requests
Activity
jonas-schievink commentedon Aug 12, 2017
The "after" column is faster than "before", did you swap them by accident?
hanna-kruppe commentedon Aug 12, 2017
It's probably because the test case is almost entirely composed of float literals, and as of #43554 those literals are parsed twice (once with
core::num::dec2flt
, once with APFloat) as a sanity check.alexcrichton commentedon Aug 12, 2017
@jonas-schievink oops yes
eddyb commentedon Aug 13, 2017
Only constant checking and MIR building should do the parsing though. Expansion, for example, couldn't possibly be affected - I suspect there's more than one factor hiding in here.
eddyb commentedon Aug 13, 2017
Another thing is that "const checking" is quadratic in the number of expression nodes (which are considered to be constant) that are part of the same subtree, as const-evaluation isn't cached (yet).
I could try doing that (and/or cache the results of parsing float literals).
nikomatsakis commentedon Aug 17, 2017
triage: P-high
Assigning to @eddyb to decide if there is something to be concerned about here or not. =)
eddyb commentedon Aug 18, 2017
On the graph "expansion" has a small random jump from
0.25
to0.28
but then it goes back down to0.25
, so that's probably a false positive elsewhere too.The culprits (adding up to most of the total change):
0.85
->3.48
1.75
->4.51
Sadly, my idea for caching const-evaluation wouldn't do much for MIR building, have to look further.
Auto merge of #44051 - eddyb:apfloat-faster-div, r=nagisa
eddyb commentedon Aug 25, 2017
The graph looks pretty good - still about half a second more, but the regression is much less worse.
@alexcrichton Are you satisfied with this outcome? Or do you want to reopen the issue?
alexcrichton commentedon Aug 25, 2017
Oh no I was personally satisfied with any outcome, I just figured it'd be good to track regressions as they came up!
Thanks for the fix @eddyb!
shepmaster commentedon Aug 25, 2017
IMO, if reopening the issue makes @eddyb perform another "5x speed increase" then we should trick them into doing such 😇