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

Increase in compilation times with newest beta & nightly #67454

Closed
fengalin opened this issue Dec 20, 2019 · 5 comments · Fixed by #67471
Closed

Increase in compilation times with newest beta & nightly #67454

fengalin opened this issue Dec 20, 2019 · 5 comments · Fixed by #67471
Labels
I-compiletime Issue: Problems and improvements with respect to compile times. regression-from-stable-to-beta Performance or correctness regression from stable to beta. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@fengalin
Copy link

Description

This was initially reported in Marwes/combine#278.

I hit an important increase in compilation times on beta & nightly with a
particular configuration using combine-3.8.1 since the recent version updates
in the Rust toolchains.

I tried building an example without combine in order to submit it to rustc
but it turns out not to be trivial. I ran into this as a side effect in CI and
can't spend much time investigating ATM, sorry.

From the CI logs I have at hand, the issue was not present as of:
rustc 1.41.0-nightly (fdc0011 2019-12-02).

Reduced example

Here is an example with which I was able to reproduce the issue:

use combine::byte::hex_digit;
use combine::{choice, token};
use combine::{ParseError, Parser, RangeStream};

fn parse<'a, I: 'a>() -> impl Parser<Input = I, Output = u8>
where
    I: RangeStream<Item = u8, Range = &'a [u8]>,
    I::Error: ParseError<I::Item, I::Range, I::Position>,
{
    choice!(
        token(b'A').map(|_| 1),
        token(b'B').map(|_| 2),
        token(b'C').map(|_| 3),
        token(b'D').map(|_| 4),
        token(b'E').map(|_| 5),
        token(b'F').map(|_| 6),
        token(b'G').map(|_| 7),
        token(b'H').map(|_| 8),
        token(b'I').map(|_| 9),
        token(b'J').map(|_| 10),
        token(b'K').map(|_| 11),
        token(b'L').map(|_| 12),
        token(b'M').map(|_| 13),
        token(b'N').map(|_| 14),
        token(b'O').map(|_| 15),
        token(b'P').map(|_| 16),
        token(b'Q').map(|_| 17),
//        token(b'R').map(|_| 18)
        (hex_digit(), hex_digit()).map(|(u, l)| {
            let val = (u << 4) | l;
            val
        })
    )
    .message("while parsing")
}

Toolchain versions

The toolchain used for these tests are those available as of 2019/12/20:

  • stable: rustc 1.40.0 (73528e3 2019-12-16)
  • beta: rustc 1.41.0-beta.1 (eb3f7c2 2019-12-17)
  • nightly: rustc 1.42.0-nightly (0de96d3 2019-12-19)

Compilation times

For each test, cargo clean was run before building in debug mode.

With message(..)

Configuration stable beta nightly
15 tokens 12.79s 26.83s 25.38s
17 tokens 12.69s 2m 20s 2m 20s
18 tokens 12.89s 6m 50s 6m 55s
17 tokens + (hex_digit, hex_digit) 12.84s 15m 57s 16m 54s

Without message(..)

"Interestingly", removing the .message(..) line reduces compilation times
significantly.

Configuration stable beta nightly
15 tokens 12.67s 19.89s 16.14s
18 tokens 12.67s 4m 11s 2m 34s
17 tokens + (hex_digit, hex_digit) 12.78s 5m 00s 4m 35s
@GuillaumeGomez GuillaumeGomez added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 20, 2019
@Centril Centril added I-compiletime Issue: Problems and improvements with respect to compile times. E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc labels Dec 20, 2019
@sdroege
Copy link
Contributor

sdroege commented Dec 20, 2019

Thanks to cargo-bisect-rustc, the first nightly with the regression is nightly-2019-12-14. nightly-2019-12-13 was still working fine.

@sdroege
Copy link
Contributor

sdroege commented Dec 20, 2019

I.e. somewhere between 3eeb8d4 and ff15e96

@sdroege
Copy link
Contributor

sdroege commented Dec 20, 2019

And we have a winner:

[...]
searched toolchains 3eeb8d4 through ff15e96
regression in 9409c20

I.e. #66405

@GuillaumeGomez GuillaumeGomez removed the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Dec 20, 2019
@nnethercote
Copy link
Contributor

Thanks for the report! I'm about to go on vacation for 6 weeks, so I will file a PR to back out #66405 before I go.

@sdroege
Copy link
Contributor

sdroege commented Dec 20, 2019

Thanks, and enjoy your holidays!

bors added a commit that referenced this issue Jan 1, 2020
Revert parts of #66405.

Because PR #66405 caused major performance regressions in some cases.

That PR had five commits, two of which affected performance, and three
of which were refactorings. This change undoes the performance-affecting
changes, while keeping the refactorings in place.

Fixes #67454.

r? @nikomatsakis
@bors bors closed this as completed in 18a3669 Jan 1, 2020
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this issue Jan 23, 2020
Because it caused major performance regressions in some cases.

That PR had five commits, two of which affected performance, and three
of which were refactorings. This change undoes the performance-affecting
changes, while keeping the refactorings in place.

Fixes rust-lang#67454.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-compiletime Issue: Problems and improvements with respect to compile times. regression-from-stable-to-beta Performance or correctness regression from stable to beta. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants