Avoid many CrateConfig clones. #37161

Merged
merged 1 commit into from Oct 19, 2016

Conversation

Projects
None yet
4 participants
@nnethercote
Contributor

nnethercote commented Oct 14, 2016

This commit changes ExtCtx::cfg() so it returns a CrateConfig
reference instead of a clone. As a result, it also changes all of the
cfg() callsites to explicitly clone... except one, because the commit
also changes macro_parser::parse() to take &CrateConfig. This is
good, because that function can be hot, and CrateConfig is expensive
to clone.

This change almost halves the number of heap allocations done by rustc
for html5ever in rustc-benchmarks suite, which makes compilation 1.20x
faster.

r? @nrc

Avoid many CrateConfig clones.
This commit changes `ExtCtx::cfg()` so it returns a `CrateConfig`
reference instead of a clone. As a result, it also changes all of the
`cfg()` callsites to explicitly clone... except one, because the commit
also changes `macro_parser::parse()` to take `&CrateConfig`. This is
good, because that function can be hot, and `CrateConfig` is expensive
to clone.

This change almost halves the number of heap allocations done by rustc
for `html5ever` in rustc-benchmarks suite, which makes compilation 1.20x
faster.
@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Oct 14, 2016

Member

If we can't use references in some cases but the data is always immutable, why not wrap it in Rc?

Member

eddyb commented Oct 14, 2016

If we can't use references in some cases but the data is always immutable, why not wrap it in Rc?

@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Oct 14, 2016

Contributor

I tried making it a &'a CrateConfig throughout, similar to sess, but there were some tricky bits that I couldn't work get to work. And this change is very simple and gets 99% of the benefit.

Contributor

nnethercote commented Oct 14, 2016

I tried making it a &'a CrateConfig throughout, similar to sess, but there were some tricky bits that I couldn't work get to work. And this change is very simple and gets 99% of the benefit.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Oct 14, 2016

Member

I mean, either Rc<CrateConfig> or using Rc inside CrateConfig should be at most as invasive, if not less, and handle all the cases so no data is ever cloned. This may improve a lot of crates' expansion.

Member

eddyb commented Oct 14, 2016

I mean, either Rc<CrateConfig> or using Rc inside CrateConfig should be at most as invasive, if not less, and handle all the cases so no data is ever cloned. This may improve a lot of crates' expansion.

@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Oct 14, 2016

Contributor

This may improve a lot of crates' expansion.

I don't understand what you mean by this. Can you elaborate? Thanks.

Contributor

nnethercote commented Oct 14, 2016

This may improve a lot of crates' expansion.

I don't understand what you mean by this. Can you elaborate? Thanks.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Oct 14, 2016

Member

I mean that there's still many clones and some of them may contribute to expansion times in other crates.
In principle, if fixing all cases is possible, it should be done, instead of specializing for one profile.

Member

eddyb commented Oct 14, 2016

I mean that there's still many clones and some of them may contribute to expansion times in other crates.
In principle, if fixing all cases is possible, it should be done, instead of specializing for one profile.

@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Oct 14, 2016

Contributor

In principle, if fixing all cases is possible, it should be done, instead of specializing for one profile.

Premature optimization, don't let perfect be the enemy of good, etc, etc. Let's see what @nrc thinks.

Contributor

nnethercote commented Oct 14, 2016

In principle, if fixing all cases is possible, it should be done, instead of specializing for one profile.

Premature optimization, don't let perfect be the enemy of good, etc, etc. Let's see what @nrc thinks.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Oct 14, 2016

Member

Hey I thought the motto of Rust was "Don't let 'good enough' be the enemy of 'perfect'" 😆

Member

eddyb commented Oct 14, 2016

Hey I thought the motto of Rust was "Don't let 'good enough' be the enemy of 'perfect'" 😆

@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Oct 16, 2016

Contributor

Hey I thought the motto of Rust was "Don't let 'good enough' be the enemy of 'perfect'" 😆

I know you are at least partly joking, but I will respond seriously: an uncompromising attitude like that makes sense for aspects of Rust's design as a language -- e.g. no compromise on memory safety -- but I don't think the same attitude is necessarily relevant or helpful when it comes to the compiler implementation.

Contributor

nnethercote commented Oct 16, 2016

Hey I thought the motto of Rust was "Don't let 'good enough' be the enemy of 'perfect'" 😆

I know you are at least partly joking, but I will respond seriously: an uncompromising attitude like that makes sense for aspects of Rust's design as a language -- e.g. no compromise on memory safety -- but I don't think the same attitude is necessarily relevant or helpful when it comes to the compiler implementation.

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Oct 18, 2016

Member

@bors: r+

It would be interesting to try @eddyb's Rc approach, but this PR makes things strictly better than they are at the moment, so no reason not to land it.

Member

nrc commented Oct 18, 2016

@bors: r+

It would be interesting to try @eddyb's Rc approach, but this PR makes things strictly better than they are at the moment, so no reason not to land it.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Oct 18, 2016

Contributor

📌 Commit 029dcee has been approved by nrc

Contributor

bors commented Oct 18, 2016

📌 Commit 029dcee has been approved by nrc

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Oct 18, 2016

Member

@nrc This PR adds clone calls all over the place, using Rc would be simpler, that's why I mentioned it.

Member

eddyb commented Oct 18, 2016

@nrc This PR adds clone calls all over the place, using Rc would be simpler, that's why I mentioned it.

eddyb added a commit to eddyb/rust that referenced this pull request Oct 18, 2016

Rollup merge of #37161 - nnethercote:no-cfg-cloning, r=nrc
Avoid many CrateConfig clones.

This commit changes `ExtCtx::cfg()` so it returns a `CrateConfig`
reference instead of a clone. As a result, it also changes all of the
`cfg()` callsites to explicitly clone... except one, because the commit
also changes `macro_parser::parse()` to take `&CrateConfig`. This is
good, because that function can be hot, and `CrateConfig` is expensive
to clone.

This change almost halves the number of heap allocations done by rustc
for `html5ever` in rustc-benchmarks suite, which makes compilation 1.20x
faster.

r? @nrc

@eddyb eddyb referenced this pull request Oct 18, 2016

Closed

Rollup of 16 pull requests #37262

bors added a commit that referenced this pull request Oct 18, 2016

Auto merge of #37262 - eddyb:rollup, r=eddyb
Rollup of 16 pull requests

- Successful merges: #36964, #37117, #37124, #37161, #37193, #37198, #37202, #37208, #37218, #37220, #37221, #37224, #37231, #37233, #37240, #37257
- Failed merges: #37213

eddyb added a commit to eddyb/rust that referenced this pull request Oct 19, 2016

Rollup merge of #37161 - nnethercote:no-cfg-cloning, r=nrc
Avoid many CrateConfig clones.

This commit changes `ExtCtx::cfg()` so it returns a `CrateConfig`
reference instead of a clone. As a result, it also changes all of the
`cfg()` callsites to explicitly clone... except one, because the commit
also changes `macro_parser::parse()` to take `&CrateConfig`. This is
good, because that function can be hot, and `CrateConfig` is expensive
to clone.

This change almost halves the number of heap allocations done by rustc
for `html5ever` in rustc-benchmarks suite, which makes compilation 1.20x
faster.

r? @nrc

bors added a commit that referenced this pull request Oct 19, 2016

Auto merge of #37262 - eddyb:rollup, r=eddyb
Rollup of 16 pull requests

- Successful merges: #36964, #37117, #37124, #37161, #37193, #37198, #37202, #37208, #37218, #37220, #37221, #37224, #37231, #37233, #37240, #37257
- Failed merges: #37213

eddyb added a commit to eddyb/rust that referenced this pull request Oct 19, 2016

Rollup merge of #37161 - nnethercote:no-cfg-cloning, r=nrc
Avoid many CrateConfig clones.

This commit changes `ExtCtx::cfg()` so it returns a `CrateConfig`
reference instead of a clone. As a result, it also changes all of the
`cfg()` callsites to explicitly clone... except one, because the commit
also changes `macro_parser::parse()` to take `&CrateConfig`. This is
good, because that function can be hot, and `CrateConfig` is expensive
to clone.

This change almost halves the number of heap allocations done by rustc
for `html5ever` in rustc-benchmarks suite, which makes compilation 1.20x
faster.

r? @nrc

@eddyb eddyb referenced this pull request Oct 19, 2016

Merged

Rollup of 23 pull requests #37269

bors added a commit that referenced this pull request Oct 19, 2016

@bors bors merged commit 029dcee into rust-lang:master Oct 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nnethercote nnethercote deleted the nnethercote:no-cfg-cloning branch Oct 19, 2016

@nnethercote

This comment has been minimized.

Show comment
Hide comment
@nnethercote

nnethercote Oct 27, 2016

Contributor

It would be interesting to try @eddyb's Rc approach

@jseyfried ended up removing all the CrateConfig clones a different (and better) way in #37431.

Contributor

nnethercote commented Oct 27, 2016

It would be interesting to try @eddyb's Rc approach

@jseyfried ended up removing all the CrateConfig clones a different (and better) way in #37431.

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