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.

Sign up for GitHub

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

Jump to bottom

Nightly Compile Time Regression (06/23 to 07/01) #34630

Closed
steffengy opened this issue Jul 3, 2016 · 8 comments · Fixed by #34652
Closed

Nightly Compile Time Regression (06/23 to 07/01) #34630

steffengy opened this issue Jul 3, 2016 · 8 comments · Fixed by #34652
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@steffengy
Copy link
Contributor

I noticed a pretty huge compile time regression from
rustc 1.11.0-nightly (ad7fe6521 2016-06-23) to rustc 1.11.0-nightly (01411937f 2016-07-01).

- time: 7.904; rss: 120MB   expansion
+ time: 84.063; rss: 121MB  expansion

Compiling the same project, results in a pretty big increase in expansion time.
I could notice similar (about 11x) slow-downs on my windows installation and
reproduced it on a fresh ubuntu.

All instructions used can be found here: https://gist.github.com/steffengy/ab3660ec9d6eecb0fc38e7d536d9e7ef
@nagisa
Copy link
Member

nagisa commented Jul 3, 2016

cc @jseyfried could be your macro-related changes.

@steffengy
Copy link
Contributor Author

It's pretty macro-heavy, so that would actually make sense.

@alexcrichton
Copy link
Member

triage: I-nominated

@alexcrichton alexcrichton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 3, 2016
@jonas-schievink
Copy link
Contributor

cc me (want to check this on my assembler macro)

@Aatch
Copy link
Contributor

Aatch commented Jul 4, 2016

The project in question is steffengy/pesty-php, for reference.

@jseyfried
Copy link
Contributor

I'll look into it.

@steffengy
Copy link
Contributor Author

steffengy commented Jul 4, 2016 •
edited
Loading

@jseyfried bisect shows that the first bad commit is 5bf7970

@jseyfried jseyfried mentioned this issue Jul 5, 2016
Merged
@jseyfried
Copy link
Contributor

@steffengy Thanks for bisecting -- I fixed this in #34652.

bors added a commit that referenced this issue Jul 6, 2016
@bors
Auto merge of #34652 - jseyfried:fix_expansion_perf, r=nrc
70e1a95 
Fix expansion performance regression

**syntax-[breaking-change] cc #31645**

This fixes #34630 by reverting commit 5bf7970 of PR #33943, which landed in #34424.

By removing the `Rc<_>` wrapping around `Delimited` and `SequenceRepetition` in `TokenTree`, 5bf7970 made cloning `TokenTree`s more expensive. While this had no measurable performance impact on the compiler's crates, it caused an order of magnitude performance regression on some macro-heavy code in the wild. I believe this is due to clones of `TokenTree`s in `macro_parser.rs` and/or `macro_rules.rs`.

r? @nrc
bors added a commit that referenced this issue Jul 7, 2016
@bors
Auto merge of #34652 - jseyfried:fix_expansion_perf, r=nrc
de78655 
Fix expansion performance regression

**syntax-[breaking-change] cc #31645**

This fixes #34630 by reverting commit 5bf7970 of PR #33943, which landed in #34424.

By removing the `Rc<_>` wrapping around `Delimited` and `SequenceRepetition` in `TokenTree`, 5bf7970 made cloning `TokenTree`s more expensive. While this had no measurable performance impact on the compiler's crates, it caused an order of magnitude performance regression on some macro-heavy code in the wild. I believe this is due to clones of `TokenTree`s in `macro_parser.rs` and/or `macro_rules.rs`.

r? @nrc
@jseyfried jseyfried mentioned this issue Oct 16, 2016
Merged
eddyb added a commit to eddyb/rust that referenced this issue Oct 19, 2016
@eddyb
Rollup merge of rust-lang#37208 - jseyfried:fix_partially_consumed_to…
e7b1d4a 
…kens_in_macros, r=nrc

macros: fix partially consumed tokens in macro matchers

Fixes rust-lang#37175.

This PR also avoids re-transcribing the tokens consumed by a matcher (and cloning the `TtReader` once per matcher), which improves expansion performance of the test case from rust-lang#34630 by ~8%.

r? @nrc
eddyb added a commit to eddyb/rust that referenced this issue Oct 19, 2016
@eddyb
Rollup merge of rust-lang#37208 - jseyfried:fix_partially_consumed_to…
094eaf0 
…kens_in_macros, r=nrc

macros: fix partially consumed tokens in macro matchers

Fixes rust-lang#37175.

This PR also avoids re-transcribing the tokens consumed by a matcher (and cloning the `TtReader` once per matcher), which improves expansion performance of the test case from rust-lang#34630 by ~8%.

r? @nrc
@jseyfried jseyfried mentioned this issue Nov 3, 2016
Merged
alexcrichton added a commit to alexcrichton/rust that referenced this issue Nov 4, 2016
@alexcrichton
Rollup merge of rust-lang#37569 - jseyfried:improve_expansion_perf, r…
bcd1e13 
…=eddyb

macros: improve expansion performance

This PR fixes that regression, further improves performance on recursive, `tt`-heavy workloads, and makes a variety of other improvements to parsing and expansion performance.

Expansion performance improvements:

| Test case      | Run-time | Memory usage |
| -------------- | -------- | ------------ |
| libsyntax      | 8%       | 10%          |
| librustc       | 15%      | 6%           |
| librustc_trans | 30%      | 6%           |
| rust-lang#37074         | 20%      | 15%          |
| rust-lang#34630         | 40%      | 8%           |

r? @eddyb
alexcrichton added a commit to alexcrichton/rust that referenced this issue Nov 5, 2016
@alexcrichton
Rollup merge of rust-lang#37569 - jseyfried:improve_expansion_perf, r…
b9cbbe1 
…=eddyb

macros: improve expansion performance

This PR fixes that regression, further improves performance on recursive, `tt`-heavy workloads, and makes a variety of other improvements to parsing and expansion performance.

Expansion performance improvements:

| Test case      | Run-time | Memory usage |
| -------------- | -------- | ------------ |
| libsyntax      | 8%       | 10%          |
| librustc       | 15%      | 6%           |
| librustc_trans | 30%      | 6%           |
| rust-lang#37074         | 20%      | 15%          |
| rust-lang#34630         | 40%      | 8%           |

r? @eddyb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix expansion performance regression jseyfried/rust
7 participants
@alexcrichton @Aatch @nagisa @steffengy @jonas-schievink @rust-highfive @jseyfried