Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

macros: improve expansion performance #37569

Merged
merged 10 commits into from
Nov 6, 2016

Conversation

jseyfried
Copy link
Contributor

@jseyfried jseyfried commented Nov 3, 2016

#34908 regressed expansion performance on recursive, tt-heavy workloads.
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%
#37074 20% 15%
#34630 40% 8%

r? @eddyb

@jseyfried
Copy link
Contributor Author

jseyfried commented Nov 3, 2016

The ast::ExprKind commit is the AST version of #37445 (h/t @nnethercote).
cc @durka @nrc

@eddyb
Copy link
Member

eddyb commented Nov 3, 2016

@Manishearth r=me

@Manishearth
Copy link
Member

@bors r=eddyb

cc @dtolnay

@bors
Copy link
Contributor

bors commented Nov 3, 2016

📌 Commit e652060 has been approved by eddyb

@jseyfried jseyfried force-pushed the improve_expansion_perf branch from e652060 to 91c778a Compare November 3, 2016 23:48
@jseyfried jseyfried force-pushed the improve_expansion_perf branch from 91c778a to b7eed53 Compare November 4, 2016 02:40
@bluss
Copy link
Member

bluss commented Nov 4, 2016

Since not needing ast coercion is a feature, it should have tests so that it is not broken by this improvement. Are there any tests? I didn't find any in the tree.

@jseyfried
Copy link
Contributor Author

jseyfried commented Nov 4, 2016

By "not needing ast coercion", do you mean #22819? That is tested here.

@bluss
Copy link
Member

bluss commented Nov 4, 2016

great! I overlooked compile-fail.

@jseyfried
Copy link
Contributor Author

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Nov 4, 2016

📌 Commit 51104e5 has been approved by eddyb

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Nov 4, 2016
…=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
bors added a commit that referenced this pull request Nov 5, 2016
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Nov 5, 2016
…=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
bors added a commit that referenced this pull request Nov 6, 2016
@jseyfried jseyfried deleted the improve_expansion_perf branch November 6, 2016 09:27
@bors bors merged commit 51104e5 into rust-lang:master Nov 6, 2016
@nnethercote
Copy link
Contributor

AFAICT this PR regressed the speed of html5ever compilation in rustc-benchmarks. perf.rust-lang.org is showing a 6% regression for html5ever this week while other benchmarks are basically unchanged. This PR is one of the PRs that show up in the regressing measurement, and I have profiling data that corroborates.

One cause of this is the span_to_filename call in Parser::new_with_doc_flag, which causes 800k extra allocations. Another cause is an extra 800k allocations in macro_parser::parse. I think this is because of the Box::new(rdr) at the top, but I'm not certain. (I found these by comparing DHAT output for before and after.)

I think these are not the only parts of the change that lead to regressions, but it's less obvious from my profiling what else might be a factor, other than vec and slice operations are showing up higher in the profile. (I got that from the Cachegrind output.)

@jseyfried
Copy link
Contributor Author

@nnethercote Thanks, I'll fix the regression ASAP.

bors added a commit that referenced this pull request Nov 14, 2016
…fried

Macro parser performance improvements and refactoring

This PR locally increased performance of #37074 by ~6.6 minutes.

Follow up to #37569, but doesn't focus explicitly on expansion performance.

History is relatively clean, but I can/will do some more polishing if this is deemed mergeable. Partially posting this now so I can get Travis to run tests for me.

r? @jseyfried
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants